Replaced CaptureScreenshot with `document.activeElement.href` #3

daneroo · 2020-01-01T22:59:05Z

This replaces the implementation of navToEnd which was using CaptureScreenshot to detect scrolling to the end of the Main/Album page. It is replaced with a query to the DOM.

When in the Main/Album Page, the DOM contains <a href=".." /> elements for all visible images.
lastPhotoInDOM() simply returns the last such href in document order.

The DOM actually contains more images than those that are visible, in a kind of virtual scrolling window.

In the DOM, but not reflecting exactly the visible photos (actually a superset of the visible elements):

<a href="./photo/AF1QipAAAAAA" aria=label="Photo - Portrait - Jul 15, 2010, 2:10:48 PM"/>
<a href="./photo/AF1QipBBBBBB" aria-label="Photo - Landscape - Jul 15, 2010, 2:03:10 PM"/>
<a href="./photo/AF1QipCCCCCC" aria-label="Video - Landscape - Jul 30, 2010, 7:20:22 PM"/>
...

We tried to find the actual oldest photo by using the aria-label attribute which contains a date for the photo, unfortunately, that label is localised for each user's language which makes the date format very hard to parse.

mpl

this will be great, thanks.

main.go

mpl · 2020-01-03T17:48:05Z

main.go

+	// The element must be focused, so that navToLast can send "\n" to enter photo detail page
+	lastEltSel := fmt.Sprintf(`a[href="%s"]`, previousHref)
+	if err := chromedp.Focus(lastEltSel).Do(ctx); err != nil {
+		log.Printf("Error focus: %s", lastEltSel)


let's return this error instead of just logging it. The caller will log it.

Agreed. I will push the change.

mpl · 2020-01-03T17:49:51Z

main.go

+	sel := `a[href^="./photo/"]` // css selector for all links to images with href prefix "./photo/..."
+	var attrs []map[string]string
+	if err := chromedp.AttributesAll(sel, &attrs).Do(ctx); err != nil {
+		log.Printf("lastPhotoInDOM: document.quertSelectorAll:%s error %s", sel, err)


if you want to add that context, just add it to the returned error, as you've done below at L337? And let the caller be in charge of the logging.

main.go

mpl · 2020-01-03T17:54:03Z

main.go

@@ -377,7 +391,8 @@ func doRun(filePath string) error {
 // navLeft navigates to the next item to the left
 func navLeft(ctx context.Context) error {
 	chromedp.KeyEvent(kb.ArrowLeft).Do(ctx)
-	chromedp.WaitReady("body", chromedp.ByQuery)
+	// Could wait for the location to change instead of this Sleep.
+	time.Sleep(200 * time.Millisecond)


yeah, I don't think we want arbitrary duration sleeps like that (as it's flaky, their effectiveness varying depending on the conditions), except when in a loop, when retrying things.

I agree, I removed the WaitReady("body",..) because it had no effect,

But I found that I needed a small delay, or my main loop would exit too soon, too often.

I did not want to tackle the arbitrary Sleep(s) in this PR, but I would like to propose an event based aproach. I have implemented this in a separate re-implmentation (node.js+puppeteer) which works really well.

For navigation: we can use chromedp.ListenTarget and get a callback when page.EventNavigatedWithinDocument or target.EventTargetInfoChanged occur, this is more reliable, and is much tighter from a timing perspective.

For download initiation: Similarly, there is a page.EventDownloadWillBegin event which can obviate the directory watching loop in (*Session) download()

If you agree I can make a separate pull request to demonstrate this idea.

Sure, I'm very interested to see your approaches in another PR. But in the meantime, let's leave navLeft as it was. The fact that navLeft sometimes does not work for you is unrelated to the change that removes the screenshots, right?

So here's my empirical testing on this point:

If I remove your time.Sleep, I have the problem that it only gets one photo, and then it stops.

If re-add your time.Sleep, it indeed seems to help (but we don't want that).

If I re-add instead the previous code (i.e. chromedp.WaitReady("body", chromedp.ByQuery)), it also seems to work. So if what you say is true (that chromedp.WaitReady("body", chromedp.ByQuery) does nothing), maybe it still has the fortunate side-effect of slowing things down (and making navLeft work) just because we're spending some CPU cycles doing "something"?

Ok, I agree, that arbitray Sleep is bad, so until we have a better condition to wait for, I will restore navLeft as it was.

daneroo · 2020-01-03T22:31:43Z

I have pushed the two requested changes. (commit: Prefer caller to log errors)

Thanks for your patience, I am just getting used to working remotely, and in smaller increments!
I see the value.
I also have the -headless PR ready to go. As soon as we merge this, I'll rebase, test and submit

Finally: Bonne année!

mpl · 2020-01-06T00:14:58Z

I have pushed the two requested changes. (commit: Prefer caller to log errors)

Thanks for your patience, I am just getting used to working remotely, and in smaller increments!
I see the value.

yes, the smaller they are, the greater the chance I find a small slice on time to review one and that we make progress on them.
I was busy again this week-end, but I should have more time starting in a couple of days and thereafter.

I also have the -headless PR ready to go. As soon as we merge this, I'll rebase, test and submit

excellent

Finally: Bonne année!

Bonne année, bonne santé.

daneroo

I pushed the commit to restore navLeft

mpl · 2020-01-10T18:06:36Z

main.go

+	// Now that we have stopped scrolling, select (focus) on the last element
+	// The element must be focused, so that navToLast can send "\n" to enter photo detail page
+	lastEltSel := fmt.Sprintf(`a[href="%s"]`, previousHref)
+	if err := chromedp.Focus(lastEltSel).Do(ctx); err != nil {
+		return err


I've just realized; doesn't this actually do the job of navToLast (except the actual navigation itself)? So couldn't we move that code to inside navToLast, so that it replaces there the loop that relies on the location change? And then we would only have to do one "\n" key event right after?

Alternatively, we could keep it here, and remove most of the the code in navToLast (it would just do the one "\n" event basically, but I don't like this solution as much as it does not separate as well each task.

Yes, we could structure it that way.
but I also prefer keeping the tasks separate.

In my other experiment, I am trying to abstract the following:

positioning: AlbumPage[First|Last],DetailPage[FirstLast]

iterating: AlbumPage[Forward|Reverse], DetailPage[Forward|Reverse]

itemCallback(item): [list,files,perkeep]

files and perkeep also have an optimistic variant which checks for existence before performing download

This way I can use the same outer loop for all combinations

The iterators are using browser events [focuschanged,targetchanged] instead of polling and arbitrary sleeps. There is also an event fired for an initiated download, which obviates the need for the directory polling.

When I'm done finding the right abstractions I can port it back to Go/cdp.

Yes, we could structure it that way.
but I also prefer keeping the tasks separate.

Hmm, I'm not sure we understood each other, so just to be clear:

do you agree that most of what navToLast is doing is redundant with and already done by L309 and L310?

if yes, do you agree to move L309 and L310 to inside navToLast?

If I'm not making sense, I can add myself a commit on top of this PR so you can see what I mean?

In my other experiment, I am trying to abstract the following:

positioning: AlbumPage[First|Last],DetailPage[FirstLast]

iterating: AlbumPage[Forward|Reverse], DetailPage[Forward|Reverse]

itemCallback(item): [list,files,perkeep]

files and perkeep also have an optimistic variant which checks for existence before performing download

This way I can use the same outer loop for all combinations

The iterators are using browser events [focuschanged,targetchanged] instead of polling and arbitrary sleeps. There is also an event fired for an initiated download, which obviates the need for the directory polling.

When I'm done finding the right abstractions I can port it back to Go/cdp.

Thanks for clarifying.
No, I don't agree that most of what navToLast is doing is redundant

We still need navToLast to work the way it does, because the selected element (309-310), is not guaranteed to be the actual last element, it is just close to the end.

Ok, understood, let's leave L309 and navToLast as they are for now.
Do you know why L309-L310 isn't enough to get the very last element though?

mpl · 2020-01-11T14:31:50Z

main.go

 	}

+	if *verboseFlag {
+		log.Printf("Successfully jumped to the end: %s", previousHref)


Hmm, then I don't think we should have previousHref in this logging message, as it might lead one to believe that previousHref is the very last element of the page (which it might not be, as you've just explained).

mpl · 2020-01-11T14:34:06Z

main.go

+	}
+
+	lastElement := attrs[len(attrs)-1]
+	href := lastElement["href"]


href isn't used by anything, so you could save yourself one variable, and just

return lastElement["href"], nil

mpl · 2020-01-11T14:35:43Z

main.go

+	// Now that we have stopped scrolling, select (focus) on the last element
+	// The element must be focused, so that navToLast can send "\n" to enter photo detail page
+	lastEltSel := fmt.Sprintf(`a[href="%s"]`, previousHref)
+	if err := chromedp.Focus(lastEltSel).Do(ctx); err != nil {
+		return err


Ok, understood, let's leave L309 and navToLast as they are for now.
Do you know why L309-L310 isn't enough to get the very last element though?

daneroo · 2020-01-11T16:17:53Z

Do you know why L309-L310 isn't enough to get the very last element though?

Yes, it is because that css selector captures more that the elements that are visible on the screen. There is a complex virtual windowing/scrolling thing going on. It's a fancy DOM version of a buffer overrun!!! Reverse engineering is sooo kludgy.

I revisited all of my assumptions and I implemented a much simpler solution.
I think you are gonna like this.

Removed the screen capture
Remove the complex css selector stuff, (yuck, never liked that, too complicated 8-))
Simply advance the selection with repeated kb.RightArrow,kb.End.
Termination criteria is when active selection stops changing.

Pushing the update to the PR now.

…veElement.href

daneroo · 2020-01-11T16:22:08Z

Ok, I think this is much better and simpler.

mpl · 2020-01-12T21:56:22Z

main.go

-			return err
+
+		if err := chromedp.Evaluate(`document.activeElement.href`, &active).Do(ctx); err != nil {
+			time.Sleep(tick) // this extra sleep is important: after the kb.End, it sometimes takes a while for the active element to be reset


so what happens if I'm on a platform with very high latency, where that sleep is not enough?
Why don't we surround L296-L299 in a for loop, so that we keep on running chromedp.Evaluate on the same element until there's no error?

Also, this comment is too long to be kept on the same line. let's move it to the line above, and format it to 80 chars.

Ok, for the comment, I will move to the line above. (I just pushed the update)

The reason the continue is preferable is that when the error condition occurs, (and enough time has passed, with the extra Sleep), the next thing that needs to happen is the kb.ArrowRight.

tl;dr

There are a lot of possible states.
As far as I can tell (having experimented a lot last night):

Initially, there is no active element, the ArrowRight will select the first photo. but the first End event does not actually perform the scroll until the ArrowRight has caused an element to be active (selected)

On a subsequent iteration, the End event causes a scroll, and then there is no active element again (this is the tricky timing part)

if the scroll has stabilized (time-sensitive), then the next kb.ArrowRight will select the first photo at the new scroll position, (this allows advancement to continue from there.

however, if the scroll has not stabilized, the ArrowRight will have the effect of reselecting the first element back at the top of the page.

Finally, after the ArrowRight has successfully selected an item at the new scroll position, all subsequent kb.End events have no effect.

So it turns that the way the loop is written is a pretty economical way of accomplishing everything we need. I did try to unroll every one of these steps, but it was less stable, and much more code.

Ok, for the comment, I will move to the line above. (I just pushed the update)

The reason the continue is preferable is that when the error condition occurs, (and enough time has passed, with the extra Sleep), the next thing that needs to happen is the kb.ArrowRight.

tl;dr

There are a lot of possible states.
As far as I can tell (having experimented a lot last night):

Initially, there is no active element, the ArrowRight will select the first photo. but the first End event does not actually perform the scroll until the ArrowRight has caused an element to be active (selected)

On a subsequent iteration, the End event causes a scroll, and then there is no active element again (this is the tricky timing part)

if the scroll has stabilized (time-sensitive), then the next kb.ArrowRight will select the first photo at the new scroll position, (this allows advancement to continue from there.

however, if the scroll has not stabilized, the ArrowRight will have the effect of reselecting the first element back at the top of the page.

I don't understand. Isn't it pretty bad to let that happen? Does this case still happen with the current patch?

Finally, after the ArrowRight has successfully selected an item at the new scroll position, all subsequent kb.End events have no effect.

So it turns that the way the loop is written is a pretty economical way of accomplishing everything we need. I did try to unroll every one of these steps, but it was less stable, and much more code.

I believe you tested that part and it works, but I'm still confused about why and how it works, so I'll have to run a few experiments myself with it when I have a bit of time.

FYI, I'm still experimenting on that part, because I don't like that we still rely on an arbitrary timeout.

Ok, looking forward to your results.
I just pushed an update (See below)

I found an algorithm that I think is simpler to understand (because it uses left arrow instead of right arrow), but it still suffers from needing one sleep, which sucks.

basically, what I want is:

var first string for { chromedp.KeyEvent(kb.ArrowLeft).Do(ctx) if err := chromedp.Evaluate(`document.activeElement.href`, &first).Do(ctx); err != nil { continue } if first == "" { continue } break } var current string for { chromedp.KeyEvent(kb.End).Do(ctx) // TODO: get rid of that sleep time.Sleep(tick) for { chromedp.KeyEvent(kb.ArrowLeft).Do(ctx) if err := chromedp.Evaluate(`document.activeElement.href`, &current).Do(ctx); err != nil { continue } break } if current == first { continue } break }

And yes, I know that does not get us to the very last element. but I don't really care, that's easy to fix. Or we can just keep the call to navToLast after, whatever.

Isn't there anything that we can do after a call to
chromedp.KeyEvent(kb.End).Do(ctx)
that would allow us to actively detect and wait for the jump to be done?

mpl · 2020-01-12T22:00:05Z

main.go

-			previousScr = scr
-			continue
+		if *verboseFlag {
+			log.Printf("** navToEnd:activeElt %s %d", active, lastRepeated)


we don't really use that kind of telegraphic style, but rather full sentences.

log.Printf("Active element %s was seen %d times", active lastRepeated)

Ok, fixed. I will keep that in mind.

mpl · 2020-01-12T22:02:04Z

main.go

 		}
-		if bytes.Equal(previousScr, scr) {
+		if lastRepeated > 2 {


why is

if lastRepeated > 1 {

not enough?

It does happen, although very rarely, that the navigation (ArrowRight) stutters (does not advance). I discovered this after running the code hundreds of times in a burn-in test.

hmm, then can you add a comment about it please?

mpl · 2020-01-12T22:02:38Z

main.go

-		previousScr = scr
-		time.Sleep(tick)
+		prev = active
+		// time.Sleep(tick)


we don't keep commented out code.

Yup, I missed that. Fixed it.

mpl · 2020-01-12T22:03:45Z

main.go

 	if *verboseFlag {
-		log.Printf("Successfully jumped to the end")
+		log.Printf("Successfully jumped to the end: %s", active)


Same comment as last time, if active is not actually the very last element.

Now that the we are using document.activeElement, and lastRepeated in our termination criteria, we are actually sure that we do have the last element.

well, then my other remark from the previous patch stands again I think ;P
i.e.: are we now not redundant with some of what navToLast does?

You are right that It is redundant with some of what navToLast does, but certainly compatible with it.

I thought we were trying to keep the change minimal (the PR is trying to replace the screenshot stuff).

Do you want to also modify navToLast to only what is essential to work with this new version of navToEnd?

You are right that It is redundant with some of what navToLast does, but certainly compatible with it.

I thought we were trying to keep the change minimal (the PR is trying to replace the screenshot stuff).

Yes, ideally we always want to keep a PR about one change and one change only, and not about several unrelated changes. However, if your fix to remove the screenshots makes navToLast obsolete and useless (which I think it does), then it's actually a good thing to remove it as part of the same PR.

Do you want to also modify navToLast to only what is essential to work with this new version of navToEnd?

Yep, or even better, let's fully remove it if we can. But if you're tired of this review, I can do it in a subsequent PR, no worries.

mpl · 2020-01-12T22:08:03Z

Do you know why L309-L310 isn't enough to get the very last element though?

Yes, it is because that css selector captures more that the elements that are visible on the screen. There is a complex virtual windowing/scrolling thing going on. It's a fancy DOM version of a buffer overrun!!! Reverse engineering is sooo kludgy.

I revisited all of my assumptions and I implemented a much simpler solution.
I think you are gonna like this.

Removed the screen capture

Remove the complex css selector stuff, (yuck, never liked that, too complicated 8-))

I don't remember if I told you, but before I did the screenshots method, one idea I had was to use the presence of their custom scrollbar on the right. As you've probably noticed, as long as you keep scrolling down, it stays visible. And if you stop, it disappears. But, when you reach the bottom of the page, even if you keep on scrolling, it eventually disappears. Unfortunately I did not find out how to "select" it.

Simply advance the selection with repeated kb.RightArrow,kb.End.

Termination criteria is when active selection stops changing.

Pushing the update to the PR now.

daneroo · 2020-01-13T00:57:14Z

Thanks for your attention to detail.

…early termination

daneroo · 2020-01-17T20:11:09Z

I just update the PR:

integrate navToLast into navToEnd
added waiting logic to navLeft (because it fails too often on first image especially)

mpl · 2020-01-20T17:51:12Z

main.go

 // navLeft navigates to the next item to the left
+// After the navigation sequence (ArrowLeft) is sent, wait for a Location change.
+// If however, after 100ms (maxIterations*miniTick), no change is seen,
+//  then return (we have reached the end)
+// navLeft almost always exits after a single iteration,
+//  but without waiting for the location change, we often see
+//  a failure to navigate, especially on the first invocation,
+//  which causes a (false) early termination of the main navN loop
 func navLeft(ctx context.Context) error {
+	var prevLocation string
+	if err := chromedp.Location(&prevLocation).Do(ctx); err != nil {
+		return err
+	}
+
 	chromedp.KeyEvent(kb.ArrowLeft).Do(ctx)
-	chromedp.WaitReady("body", chromedp.ByQuery)
+
+	maxIterations := 10
+	miniTick := 10 * time.Millisecond
+	var location string
+	for i := 0; i < maxIterations; i++ {
+
+		if err := chromedp.Location(&location).Do(ctx); err != nil {
+			return err
+		}
+		if location != prevLocation {
+			log.Printf("navLeft break at it:%d", i)
+			break
+		}
+		time.Sleep(miniTick)
+	}


let's worry about that in another PR please.

Ok, I reverted the change to navLeft.
Unfortunately, the original navLeft does not work for me, which makes testing rather hard. (it falsely terminates early too often).

I'm also a bit lost in the threaded comments. Perhaps I'm not using the review tooling correctly. If you feel this is dragging on, we can also just close the PR.

Ok, I reverted the change to navLeft.
Unfortunately, the original navLeft does not work for me, which makes testing rather hard. (it falsely terminates early too often).

Can you create another PR for that fix please? we don't have to wait for the current one to be over. It can all be done in parallel.

I'm also a bit lost in the threaded comments. Perhaps I'm not using the review tooling correctly.

I suspect it's both. We probably got carried away too many times, and github can be weird at times (I'm still much more comfortable myself in gerrit tbh). I suggest reloading the page, taking a big breath and rereading it all from the start, noting as you go along what is actually still a conversation, and what is not. :-)

If you feel this is dragging on, we can also just close the PR.

Let's not close it please. I am still hopeful that we can make one (variant) of the solutions you provided work, i.e. replacing the screenshots with another one that is less gross, but that also does not use arbitrary sleeps. But if you are bored with that problem, no worries, I'll just keep on iterating myself on that PR. And of course feel free to open other (independent) PRs in parallel.

Cool. I did a separate PR for -headless flag.
I will look at navLeft next, and I can circle back to this navToLast/End again after.

Replaced CaptureScreenshot with a DOM Query

3c668ae

daneroo mentioned this pull request Jan 1, 2020

Navigation flow hangs in navToLast() #4

Open

mpl requested changes Jan 3, 2020

View reviewed changes

Prefer caller to log errors

c4e30db

daneroo requested a review from mpl January 5, 2020 19:05

restore navLeft, removing arbitrary time.Sleep

59f6603

daneroo commented Jan 10, 2020

View reviewed changes

mpl reviewed Jan 10, 2020

View reviewed changes

mpl requested changes Jan 11, 2020

View reviewed changes

daneroo changed the title ~~Replaced CaptureScreenshot with a DOM Query~~ Replaced CaptureScreenshot with document.activeElement.href Jan 11, 2020

Remove all the css stuff, simplify with sigle loop over document.acti…

a7f9a45

…veElement.href

mpl requested changes Jan 12, 2020

View reviewed changes

daneroo added 2 commits January 12, 2020 19:40

Reformat comments

07de945

full sentences, not telegraphic style

0369c1f

daneroo added 2 commits January 17, 2020 12:31

Folded navToLast into navToEnd, also added check to navLeft to avoid …

2b17b21

…early termination

Move navToEnd logging line

c224e2f

mpl reviewed Jan 20, 2020

View reviewed changes

reverse navLeft modifiacations

e4a0f89

Replaced CaptureScreenshot with document.activeElement.href #3

Are you sure you want to change the base?

Replaced CaptureScreenshot with document.activeElement.href #3

Conversation

daneroo commented Jan 1, 2020

mpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daneroo commented Jan 3, 2020 • edited

mpl commented Jan 6, 2020

daneroo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daneroo commented Jan 11, 2020

daneroo commented Jan 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpl commented Jan 12, 2020

daneroo commented Jan 13, 2020

daneroo commented Jan 17, 2020

Choose a reason for hiding this comment

daneroo Jan 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replaced CaptureScreenshot with `document.activeElement.href` #3

Replaced CaptureScreenshot with `document.activeElement.href` #3

daneroo commented Jan 3, 2020 •

edited

daneroo Jan 21, 2020 •

edited