Remember the Outline on document refresh

[bendikro]

Expanded nodes in document outline are collapsed (back to default) when the document is refreshed.

A better behaviour would be to remember which nodes where expanded and keep to scrollbar and selection when refreshing.

[Adam Reichold (adamreichold)]

Hello,

I am sorry but this will be a Won't Fix for now since I don't think explicitly tracking the changes to the outline after a refresh that might allow us to keep the expansion state is worth the effort.

Best regards, Adam.

[bendikro (bendikro)]

Hey, I implemented a solution for this that simply saves the description for each expanded element before a refresh, and restores them right after. It will not handle duplicates well though, as it does not track depth or page numbers, but if chapter/sections includes numbering in the outline, they'll be unique though.

Pros with not tracking page numbers is that it'll still be able to remember the expansion when changing the document content so that page numbering changes.

https://github.com/bendikro/qpdfview/commit/8518b4e7654776984294e95c4a5b14f2c6c0867d
https://github.com/bendikro/qpdfview/commit/8518b4e7654776984294e95c4a5b14f2c6c0867d.patch

[Adam Reichold (adamreichold)]

Hello bendikro,

thank you for your contribution. I agree with the concept, i.e. storing displayed text instead of page number, but I see various issues with the implementation:
* If you are already registered with Launchpad, why not just submit the change as a Bazaar merge request? Git and GitHub are fine tools, but they are not the tools this project uses.
* Introducing another cyclic dependency between the DocumentView and MainWindow classes is not good, doing it via a globally accessible singleton is worse. Also exposing the related internals of the MainWindow does not help.
* I do not think we need another item data role for this, but should rather integrate it with the existing expansion role to loosen the coupling. Also, a list seems a rather inefficient data structure for the task at hand.

In any case, I created a branch which implements such functionality (slightly only extended to handle nesting) and published it at [1]. Please try it out and report here if that would fulfil your requirements as well.

Best regards, Adam.

[1] https://code.launchpad.net/~adamreichold/qpdfview/outline-after-refresh

[Adam Reichold (adamreichold)]

P.S.: I also do not see a reason why this should be limited to PDF documents...

[Adam Reichold (adamreichold)]

Hello again,

do you have a timeframe for taking a look at the branch? It also contains some general clean-up to item model usage which I would like to merge and if you just need some more time, I'll prepare them separately from this particular function.

Best regards, Adam.

[bendikro (bendikro)]

Hi Adam

I'm on vacation so I can't look at it before february. As for your comments about the implementation, I fully agree. I'm well aware of the hacks employed in the patch ;-), but as I didn't really have time to do this in the first place, I needed a quick solution to get it working.
I used git-bzr to quickly fetch the source, which is why I used git. I guess the data roles could be merged, however I think a list should be sufficiently efficient unless the outline is extremely large. It doesn't really matter to me which data structure is used though.

[Adam Reichold (adamreichold)]

Hello again,

ok, so it would take quite some time. Since the branch fixes all the mentioned implementation problems and it seems unprobable that it will intefere with other functions, I merged it into trunk just now.

Best regards, Adam.

[bendikro (bendikro)]

Hey

The changes you merged seem to work as expected. Thanks.

[bendikro (bendikro)]

The only thing that could be fixed is to also restore the scrollbar position after refresh.

[bendikro (bendikro)]

To fix the issue with the scrollbar, a reference to the outline TreeView is required, which is not available from dcoumentview. Any suggestions on how to solve that?

[Adam Reichold (adamreichold)]

Hello bendikro,

I don't think a reference to the TreeView instance is the problem here as you can just as well store the values within the model to first save/restore them on tab change and then also on refresh, but rather the problem is that scroll bar positions will be frequently invalidated when the model contents is changed and the item view adjusts its layout.

If you have a look at the linked branch, the bookkeeping is straight forward, but the question is when to call the restoral methods since very often they will restore the scroll bar position with a scroll bar that does not yet have that range available. For example, just delaying the call to "TreeView::restoreScrollBarPositions" after event processing idles using "QTimer::singleShot" does not yield predictable results.

But in any case, you can have a look at it and try to find a better way to invoke the restoral.

Best regards, Adam.

[bendikro (bendikro)]

Hi

Can you take a look at this: https://code.launchpad.net/~bendikro/qpdfview/restore-outline-scrollbars

It seems to work. I also added storing the selected entry on refresh and tab change, which is done in TreeView::saveItemExpansion/TreeView::restoreItemExpansion using m_expansionRole + 1 (which you might want to change...?)

[Adam Reichold (adamreichold)]

Hello,

I tried the code and it does work for refreshing documents when the contents of the outline does not change. But what happens when the contents does change and you reach the old maximum again after expanding new items (rather likely as the row height is uniform) and suddenly your scroll bars gets "restored"? (And again, public event handlers and helper methods and code style. Please try to be consistent with the existing code base.)

But looking at all the code necessary to accomplish this rather minor extension of the feature and registering the fact that restoring the selected item did already try to sneak in via that last commit, I do not think that I will merge this in any form as feature creep has to stop somewhere and the return on investement for all the code is minimal.

Best regards, Adam.