Clear all find highlights when the findbar is closed (issue 7468) #10100

Snuffleupagus · 2018-09-22T14:22:10Z

Please note that this will require a mozilla-central follow-up patch, e.g. looking something like this, in order for this to work in the built-in Firefox PDF viewer as well.

Fixes #7468.

timvandermeij · 2018-09-22T18:43:35Z

web/pdf_find_controller.js

@@ -53,11 +53,19 @@ class PDFFindController {

    this.reset();

+    eventBus.on('findbarclose', () => {


I think we can implement this feature without tracking any state in the find controller. We should be able to get rid of this active member variable if the text layer builder listens to the event on the event bus instead and tracks that member variable internally. After all it's a text layer state (whether or not to highlight the matches) and not find controller state (which only produces the matches and leaves the representation to the text layer builder).

In short, can we try removing all changes in this file and introducing this._highlightMatches = true; in the constructor of the text layer builder, with a listener for the findbarclose event that toggles this value, and then check this value at https://github.com/mozilla/pdf.js/pull/10100/files#diff-6a8d3c6754c77cfa0876314f0f54bed7R319?

After all it's a text layer state (whether or not to highlight the matches) and not find controller state (which only produces the matches and leaves the representation to the text layer builder).

While I cannot reasonably disagree with any of the above, given that TextLayerBuilder already have access to PDFFindController the current solution just seemed like the simplest possible one :-)

In short, can we try removing all changes in this file and introducing this._highlightMatches = true; in the constructor of the text layer builder, with a listener for the findbarclose event that toggles this value,

There's a slight snag with this suggestion, which is why I went with the current solution:
With the suggested changes there wouldn't be anything resetting this._highlightMatches for subsequent find operations, thus permanently breaking highlighting of matches once the findbar has been closed just one time.

In order for this to work, you'd basically need to start dispatching an event instead at line

pdf.js/web/pdf_find_controller.js

Line 356 in 54d6c24

this.active = true;

and have the TextLayerBuilder instances listen for it in order to set this._highlightMatches = true. That feels slightly more involved, and there's also the question of what a good event name would be; updatetextlayerhighlight perhaps?

That should probably work too, although I've not tested it, but at the expense of a bit more event dispatching though. How do you think that we should proceed here?

Having (quickly) tried using events instead, it unfortunately seems that the solution will need to become a bit more complex than current one for things to work out.
Most likely this can all be addressed in a good way, but it's not as easy as one would hope. Besides, there's also the issue described in #10099 (comment) which affects this PR as well.

My idea was indeed that in the places where we manipulate the this.active member variable we'd use events instead, so where we set this.active = true/false; (doing a search sets it to true and closing the findbar sets it to false) we dispatch an event instead (updatetextlayerhighlight seems like a good name to me) and the text layer builder sets this value for the this._highlightMatches member variable that it tracks to display the highlighted matches or not. I didn't implement it, so it may indeed be a bit more complicated than I can foresee now, but I think that should work so we don't have to do anything in the find controller.

If this does turn out to be more complicated than what I described, can we at least rename active to highlightMatches or something similar? I find that a much more descriptive name and then we don't need the extra comment behind its declaration in the reset method.

Having thought about all the various cases, the conclusion is unfortunately that we still need to track this state in PDFFindController.
To summarize: There's simply no good way to relay the correct highlight state to all TextLayerBuilder instances, since they are created lazily; please read on for for additional details.

The important point here is that you need one source of truth, and not multiple, for whether or not find highlights should be displayed. There's only ever going to be one PDFFindController instance around, but an arbitrary number of TextLayerBuilder ones (i.e. one per page).

In particular, even attempting to track this in TextLayerBuilder is basically impossible when you consider all of the cases:

Assume that you set this.highlightMatches = true in the TextLayerBuilder constructor.
That means that for currently active pages, i.e. ones in the PDFPageViewBuffer cache, you'll receive the event to disable the highlights when closing the findbar. However when new pages are scrolled into view, and thus new TextLayerBuilder instances are created, those will incorrectly have highlights enable since they didn't exist at the point when the events (regardless if those are 'findbarclose' or 'updatetextlayerhighlight') were dispatched.

Assume instead that this.highlightMatches = false is set in the TextLayerBuilder constructor, and that an event is dispatched whenever searching starts.
Similar to the above, for new TextLayerBuilder instances created after searching started, no events are received and thus highlights will be permanently disabled in this case.

Furthermore please note that I choose to piggy-back on the 'updatetextlayermatches' event from your PR, for two reasons:

First of all, there's fewer events overall that way (and the name fits nicely here as well). Note that this PR alone means that each TextLayerBuilder instance registers two event listeners, and with your PR that would have otherwise increased to a total of three events.

Second of all, by having only PDFFindController listen for 'findbarclose' events, there's no longer necessary to worry about the order in which the 'findbarclose' listeners are registered and run, thus guaranteeing that highlightMatches has been correctly set.

Yes, the lazy loading is a really good point which indeed I didn't foresee in my idea. In that case the current code structure seems fine to me!

timvandermeij · 2018-09-23T15:07:30Z

web/text_layer_builder.js

+                      'have been called when page rendering was cancelled.');
+        return;
+      }
+      for (const name in _boundEvents) {


Could you add a comment here about why this is necessary? Basically a few lines of summary from the discussion in the other PR so we don't forget the reason why we added this and don't accidentally remove this later without a better alternative.

Good point; please check that the added comment is clear enough.

Looks really good!

…hen rendering is cancelled Also, the patch updates `TextLayerBuilder` to use the new 'pagecancelled' event for (future) event removal purposes.

timvandermeij · 2018-09-24T23:15:22Z

Overall this looks good to me, but I'll do another check in the coming days to make sure I didn't miss anything. We can probably land this first (and create a follow-up issue for mozcentral) and then I can rebase and remove the largest part of the first commit from my PR (only the findbar dependency part will probably remain).

Snuffleupagus · 2018-09-25T09:48:55Z

We can probably land this first (and create a follow-up issue for mozcentral) and then I can rebase and remove the largest part of the first commit from my PR (only the findbar dependency part will probably remain).

In all fairness, since your PR was submitted earlier and it's larger too, if you want we could probably land your PR first if you just include my 'pagecancelled' commit there too.

Anyway, I'm sorry for blocking your PR like this, but I really don't think it would have been good to (even temporarily) land code that would cause the number of EventBus listeners to grow arbitrarily large.

Please note that this will require a `mozilla-central` follow-up patch, in order for this to work in the built-in Firefox PDF viewer as well.

Snuffleupagus · 2018-09-28T09:37:31Z

/botio-linux preview

pdfjsbot · 2018-09-28T09:37:32Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.67.70.0:8877/8cfd0db29365254/output.txt

pdfjsbot · 2018-09-28T09:40:30Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/8cfd0db29365254/output.txt

Total script time: 2.97 mins

Published

Viewer: http://54.67.70.0:8877/8cfd0db29365254/web/viewer.html

timvandermeij · 2018-09-30T13:35:14Z

Thank you for helping out with this!

Pull request mozilla#10100 removed the last usage of the find controller from the find bar, so we can drop the dependency now.

timvandermeij added the viewer label Sep 22, 2018

timvandermeij reviewed Sep 22, 2018

View reviewed changes

Snuffleupagus force-pushed the findbarclose branch 5 times, most recently from e86d811 to 7fd3699 Compare September 23, 2018 12:30

Snuffleupagus mentioned this pull request Sep 23, 2018

[api-major] Rework the find controller for unit testing #10099

Merged

timvandermeij reviewed Sep 23, 2018

View reviewed changes

Snuffleupagus force-pushed the findbarclose branch from 7fd3699 to d5a36da Compare September 23, 2018 20:33

Dispatch a 'pagecancelled' event, in PDFPageView.cancelRendering, w…

1eaa3b8

…hen rendering is cancelled Also, the patch updates `TextLayerBuilder` to use the new 'pagecancelled' event for (future) event removal purposes.

Snuffleupagus force-pushed the findbarclose branch from d5a36da to 05535b0 Compare September 23, 2018 20:34

Clear all find highlights when the findbar is closed (issue 7468)

f29b4d1

Please note that this will require a `mozilla-central` follow-up patch, in order for this to work in the built-in Firefox PDF viewer as well.

Snuffleupagus force-pushed the findbarclose branch from 05535b0 to f29b4d1 Compare September 26, 2018 08:21

timvandermeij approved these changes Sep 30, 2018

View reviewed changes

timvandermeij merged commit d0620ec into mozilla:master Sep 30, 2018

timvandermeij mentioned this pull request Sep 30, 2018

Implement find bar close event in mozilla-central #10115

Closed

Snuffleupagus deleted the findbarclose branch September 30, 2018 14:08

timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Sep 30, 2018

Remove the find bar's dependency on the find controller

7aca53b

Pull request mozilla#10100 removed the last usage of the find controller from the find bar, so we can drop the dependency now.

Snuffleupagus mentioned this pull request Sep 30, 2018

Fix for #7468 - Cleared all highlights when find bar is closed #7501

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear all find highlights when the findbar is closed (issue 7468) #10100

Clear all find highlights when the findbar is closed (issue 7468) #10100

Snuffleupagus commented Sep 22, 2018 •

edited

Loading

timvandermeij Sep 22, 2018 •

edited

Loading

Snuffleupagus Sep 23, 2018 •

edited

Loading

Snuffleupagus Sep 23, 2018

timvandermeij Sep 23, 2018

Snuffleupagus Sep 23, 2018 •

edited

Loading

timvandermeij Sep 24, 2018

timvandermeij Sep 23, 2018

Snuffleupagus Sep 23, 2018

timvandermeij Sep 24, 2018

timvandermeij commented Sep 24, 2018

Snuffleupagus commented Sep 25, 2018

Snuffleupagus commented Sep 28, 2018

pdfjsbot commented Sep 28, 2018

pdfjsbot commented Sep 28, 2018

timvandermeij commented Sep 30, 2018

		@@ -53,11 +53,19 @@ class PDFFindController {

		this.reset();

		eventBus.on('findbarclose', () => {

Clear all find highlights when the findbar is closed (issue 7468) #10100

Clear all find highlights when the findbar is closed (issue 7468) #10100

Conversation

Snuffleupagus commented Sep 22, 2018 • edited Loading

timvandermeij Sep 22, 2018 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Sep 23, 2018

Choose a reason for hiding this comment

timvandermeij Sep 23, 2018

Choose a reason for hiding this comment

Snuffleupagus Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

timvandermeij Sep 24, 2018

Choose a reason for hiding this comment

timvandermeij Sep 23, 2018

Choose a reason for hiding this comment

Snuffleupagus Sep 23, 2018

Choose a reason for hiding this comment

timvandermeij Sep 24, 2018

Choose a reason for hiding this comment

timvandermeij commented Sep 24, 2018

Snuffleupagus commented Sep 25, 2018

Snuffleupagus commented Sep 28, 2018

pdfjsbot commented Sep 28, 2018

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 28, 2018

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Sep 30, 2018

Snuffleupagus commented Sep 22, 2018 •

edited

Loading

timvandermeij Sep 22, 2018 •

edited

Loading

Snuffleupagus Sep 23, 2018 •

edited

Loading

Snuffleupagus Sep 23, 2018 •

edited

Loading