Dispatch a sidebarviewchanged event in PDFSidebar when the view changes
#7190
Conversation
| // Only update the storage when the document has been loaded *and* rendered. | ||
| return; | ||
| } | ||
| // store.initializedPromise.then(function() { |
There was a problem hiding this comment.
Why is this code commented?
|
There are parts of this PR, e.g. the writing to storage, that probably shouldn't land unless we also add support for getting/utilizing the @timvandermeij One option could be that I append the commit from PR #7010 here, so that it can land all at once? I'm not sure if that makes testing more difficult though, so what do you think? |
| @@ -90,9 +90,6 @@ var PDFSidebar = (function PDFSidebarClosure() { | |||
| reset: function PDFSidebar_reset() { | |||
| this.isInitialViewSet = false; | |||
|
|
|||
| this.close(); | |||
There was a problem hiding this comment.
Why were these lines removed?
There was a problem hiding this comment.
Sorry, this is probably not entirely relevant to this PR, and it concerns an edge-case anyway.
When looking through the code in PDFSidebar, it struck me that forcibly closing the sidebar during PDFViewerApplication.close() may not be what the user expects. This really only matters in default viewer, when the "Open" functionality is present.
If you feel that we shouldn't change this at all, or that it doesn't belong in this PR (which I'd totally understand!), I'll re-add these two lines!
|
I think appending the commit (with the right author information) here should be fine. I think testing actually becomes easier because it allows me to test both patches at once, also since the other commit now depends on this patch. |
…hanges We cannot piggy-back on the `updateviewarea` event in order to update the stored sidebar state, since there're a number of cases where opening/switching the sidebar view won't fire a `updateviewarea` event. Note that `updateviewarea` only fires when the position changes in the *viewer*, which means that it won't fire if e.g. the viewer is narrow, such that the sidebar overlays the document transparently; or when switching views, without the document position also changing. This patch also moves the handling of `forceOpen` parameter in `PDFSidebar_switchView`, to prevent triggering back-to-back rendering and dispatching of events.
Persist the state of content sidebar while browsing away from viewer and initializing the same on returning back to the viewer. The state is saved in persistent store preferences and used upon viewer initialization. Fixes #6935
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/103f39b80368170/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/103f39b80368170/output.txt Total script time: 1.01 mins Published |
|
Thank you! |
We cannot piggy-back on the
updateviewareaevent in order to update the stored sidebar state, since there're a number of cases where opening/switching the sidebar view won't fire aupdateviewareaevent.Note that
updateviewareaonly fires when the position changes in the viewer, which means that it won't fire if e.g. the viewer is narrow, such that the sidebar overlays the document transparently; or when switching views, without the document position also changing.This patch also moves the handling of
forceOpenparameter inPDFSidebar_switchView, to prevent triggering back-to-back rendering and dispatching of events./cc @timvandermeij This ought to take care of the issues you pointed out in #7010.
I should probably just have added the event in #7038, but since there wasn't any consumer of it back then, it simply slipped my mind.