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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Sep 30, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1eaa3b8
Dispatch a 'pagecancelled' event, in `PDFPageView.cancelRendering`, w…
Snuffleupagus Sep 23, 2018
f29b4d1
Clear all find highlights when the findbar is closed (issue 7468)
Snuffleupagus Sep 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions web/firefoxcom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,20 +169,27 @@ class MozL10n {
'findhighlightallchange',
'findcasesensitivitychange',
'findentirewordchange',
'findbarclose',
];
let handleEvent = function(evt) {
let handleEvent = function({ type, detail, }) {
if (!PDFViewerApplication.initialized) {
return;
}
if (type === 'findbarclose') {
PDFViewerApplication.eventBus.dispatch('findbarclose', {
source: window,
});
return;
}
PDFViewerApplication.eventBus.dispatch('find', {
source: window,
type: evt.type.substring('find'.length),
query: evt.detail.query,
type: type.substring('find'.length),
query: detail.query,
phraseSearch: true,
caseSensitive: !!evt.detail.caseSensitive,
entireWord: !!evt.detail.entireWord,
highlightAll: !!evt.detail.highlightAll,
findPrevious: !!evt.detail.findPrevious,
caseSensitive: !!detail.caseSensitive,
entireWord: !!detail.entireWord,
highlightAll: !!detail.highlightAll,
findPrevious: !!detail.findPrevious,
});
};

Expand Down
3 changes: 2 additions & 1 deletion web/pdf_find_bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,8 @@ class PDFFindBar {
this.opened = false;
this.toggleButton.classList.remove('toggled');
this.bar.classList.add('hidden');
this.findController.active = false;

this.eventBus.dispatch('findbarclose', { source: this, });
}

toggle() {
Expand Down
17 changes: 15 additions & 2 deletions web/pdf_find_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,24 @@ class PDFFindController {

this.reset();

eventBus.on('findbarclose', () => {
Copy link
Contributor

@timvandermeij timvandermeij Sep 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus Sep 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@Snuffleupagus Snuffleupagus Sep 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

this._highlightMatches = false;

eventBus.dispatch('updatetextlayermatches', {
source: this,
pageIndex: -1,
});
});

// Compile the regular expression for text normalization once.
const replace = Object.keys(CHARACTERS_TO_NORMALIZE).join('');
this._normalizationRegex = new RegExp(`[${replace}]`, 'g');
}

get highlightMatches() {
return this._highlightMatches;
}

get pageMatches() {
return this._pageMatches;
}
Expand All @@ -75,7 +88,7 @@ class PDFFindController {
}

reset() {
this.active = false; // If active, find results will be highlighted.
this._highlightMatches = false;
this._pageMatches = [];
this._pageMatchesLength = null;
this._state = null;
Expand Down Expand Up @@ -353,7 +366,7 @@ class PDFFindController {
const currentPageIndex = this._pdfViewer.currentPageNumber - 1;
const numPages = this._pdfViewer.pagesCount;

this.active = true;
this._highlightMatches = true;

if (this._dirtyMatch) {
// Need to recalculate the matches, reset everything.
Expand Down
10 changes: 10 additions & 0 deletions web/pdf_page_view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,8 @@ class PDFPageView {
}

cancelRendering(keepAnnotations = false) {
const renderingState = this.renderingState;

if (this.paintTask) {
this.paintTask.cancel();
this.paintTask = null;
Expand All @@ -275,6 +277,14 @@ class PDFPageView {
this.annotationLayer.cancel();
this.annotationLayer = null;
}

if (renderingState !== RenderingStates.INITIAL) {
this.eventBus.dispatch('pagecancelled', {
source: this,
pageNumber: this.id,
renderingState,
});
}
}

cssTransform(target, redrawAnnotations = false) {
Expand Down
39 changes: 38 additions & 1 deletion web/text_layer_builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ class TextLayerBuilder {
this.textLayerRenderTask = null;
this.enhanceTextSelection = enhanceTextSelection;

this._boundEvents = Object.create(null);
this._bindEvents();

this._bindMouse();
}

Expand Down Expand Up @@ -314,7 +317,7 @@ class TextLayerBuilder {
clearedUntilDivIdx = match.end.divIdx + 1;
}

if (this.findController === null || !this.findController.active) {
if (!this.findController || !this.findController.highlightMatches) {
return;
}

Expand All @@ -331,6 +334,40 @@ class TextLayerBuilder {
this.renderMatches(this.matches);
}

/**
* @private
*/
_bindEvents() {
const { eventBus, _boundEvents, } = this;

_boundEvents.pageCancelled = (evt) => {
if (evt.pageNumber !== this.pageNumber) {
return;
}
if (this.textLayerRenderTask) {
console.error('TextLayerBuilder._bindEvents: `this.cancel()` should ' +
'have been called when the page was reset, or rendering cancelled.');
return;
}
// Ensure that all event listeners are cleaned up when the page is reset,
// since re-rendering will create new `TextLayerBuilder` instances and the
// number of (stale) event listeners would otherwise grow without bound.
for (const name in _boundEvents) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks really good!

eventBus.off(name.toLowerCase(), _boundEvents[name]);
delete _boundEvents[name];
}
};
_boundEvents.updateTextLayerMatches = (evt) => {
if (evt.pageIndex !== -1) {
return;
}
this.updateMatches();
};

eventBus.on('pagecancelled', _boundEvents.pageCancelled);
eventBus.on('updatetextlayermatches', _boundEvents.updateTextLayerMatches);
}

/**
* Improves text selection by adding an additional div where the mouse was
* clicked. This reduces flickering of the content if the mouse is slowly
Expand Down