Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaced CaptureScreenshot with document.activeElement.href #3

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
84 changes: 49 additions & 35 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ limitations under the License.
package main

import (
"bytes"
"context"
"errors"
"flag"
Expand All @@ -33,7 +32,6 @@ import (
"strings"
"time"

"github.com/chromedp/cdproto/cdp"
"github.com/chromedp/cdproto/input"
"github.com/chromedp/cdproto/page"
"github.com/chromedp/chromedp"
Expand Down Expand Up @@ -280,54 +278,69 @@ func (s *Session) firstNav(ctx context.Context) error {
return nil
}

// navToEnd waits for the page to be ready to receive scroll key events, by
// trying to select an item with the right arrow key, and then scrolls down to the
// end of the page, i.e. to the oldest items.
// navToEnd waits for the page to be ready to receive scroll key events,
// and then scrolls down to the end of the page, i.e. to the oldest items.
func navToEnd(ctx context.Context) error {
// wait for page to be loaded, i.e. that we can make an element active by using
// the right arrow key.
for {
chromedp.KeyEvent(kb.ArrowRight).Do(ctx)
time.Sleep(tick)
var ids []cdp.NodeID
if err := chromedp.Run(ctx,
chromedp.NodeIDs(`document.activeElement`, &ids, chromedp.ByJSPath)); err != nil {
return err
}
if len(ids) > 0 {
if *verboseFlag {
log.Printf("We are ready, because element %v is selected", ids[0])
}
break
}
time.Sleep(tick)
}
// wait for page to be loaded
chromedp.WaitReady("body", chromedp.ByQuery).Do(ctx)

mpl marked this conversation as resolved.
Show resolved Hide resolved
// try jumping to the end of the page. detect we are there and have stopped
// moving when two consecutive screenshots are identical.
var previousScr, scr []byte
// Try scrolling to the end of the page.
// After each scroll attempt we extract the last Photo Page Element (href) from the DOM.
// We detect we are at the end when two consecutive DOM extractions are identical.
var previousHref string
for {
chromedp.KeyEvent(kb.PageDown).Do(ctx)
chromedp.KeyEvent(kb.End).Do(ctx)
chromedp.CaptureScreenshot(&scr).Do(ctx)
if previousScr == nil {
previousScr = scr
continue
time.Sleep(tick) // sleep *before* we establish new state in DOM

// Extract last Photo Page Element (href) from DOM.
lastHrefInDOM, err := lastPhotoInDOM(ctx)
if err != nil {
continue // Just ignore this error, continue will retry.
}
if bytes.Equal(previousScr, scr) {
if previousHref == lastHrefInDOM {
break
}
previousScr = scr
time.Sleep(tick)
previousHref = lastHrefInDOM
}

if *verboseFlag {
log.Printf("Successfully jumped to the end")
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. do you agree that most of what navToLast is doing is redundant with and already done by L309 and L310?
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
return nil
}

// 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.
func lastPhotoInDOM(ctx context.Context) (string, error) {
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 {
return "", err
}
if len(attrs) == 0 {
return "", fmt.Errorf("lastPhotoInDOM: no elements match")
}

lastElement := attrs[len(attrs)-1]
href := lastElement["href"]
mpl marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return lastElement["href"], nil

return href, nil
}

// navToLast sends the "\n" event until we detect that an item is loaded as a
// new page. It then sends the right arrow key event until we've reached the very
// last item.
Expand Down Expand Up @@ -552,6 +565,7 @@ func (s *Session) navN(N int) func(context.Context) error {
if N > 0 && n >= N {
break
}

if err := navLeft(ctx); err != nil {
return err
}
Expand Down