[Okular-devel] Review Request 122570: Update session restore/save to account for multiple tabs

[Jonathan Doman]

> On Feb. 22, 2015, 10:28 a.m., Albert Astals Cid wrote:
> > Was thinking about the saving the viewport, are we closing the documents properly? Because if we are, on opening they should just be resotred to their proper viewport as when just opening a document from scratch, no?

Yes, that's what I've been saying. This change properly closes the documents so that it's not necessary to save viewports in the session config.


> On Feb. 22, 2015, 10:28 a.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 680
> > <https://git.reviewboard.kde.org/r/122570/diff/2/?file=350709#file350709line680>
> >
> >     Do we need to call slotQuit both from here and from the signal connection? I understand you mean session only calls aboutToQuit and not the closeEvent (which is weird), but does normal close only call closeEvent and not aboutToQuit?

Yes, there are two different paths:
1. Window is closed by user. This triggers closeEvent (and calls destructor). Once windows are gone, app quits and then aboutToQuit is triggered.
2. Session manager closes application (through QCoreApplication::quit() I think). This just causes app.exec() to return - windows are not closed nor are destructors called. So the only way to respond is to connect to aboutToQuit.

Just in case this changes in the future, we can clear the tab list in slotQuit so that it's okay to call twice.


- Jonathan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122570/#review76430
-----------------------------------------------------------


On Feb. 22, 2015, 2:54 p.m., Jonathan Doman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122570/
> -----------------------------------------------------------
> 
> (Updated Feb. 22, 2015, 2:54 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 335852
>     http://bugs.kde.org/show_bug.cgi?id=335852
> 
> 
> Repository: okular
> 
> 
> Description
> -------
> 
> New Shell logic loops through each tab and saves URLs and active tab index in session config.
> 
> Viewport was previously saved in session config, but I opted to remove it because:
> 1. It complicates the restore logic. It would require either using QMetaObject::invoke or adding functions to ViewerInterface. Also hard to get right since opening a document isn't exactly synchronous.
> 2. Viewport info is already saved during a graceful shutdown.
> 
> 
> Diffs
> -----
> 
>   part.h 594eb44113ae130a6fefbf2800af32886aa3cbef 
>   part.cpp 36438af1cd1036ee954f80b5359a0cab2c019036 
>   shell/main.cpp 16289608f0acf299db04258d842bbb87add62c0b 
>   shell/shell.h 224acfe023ef8e9cc58b52ddf32068af8937896a 
>   shell/shell.cpp f7675fdc8203e90210b8ba82620b19ae69ee43e1 
> 
> Diff: https://git.reviewboard.kde.org/r/122570/diff/
> 
> 
> Testing
> -------
> 
> I was not familiar with session functionality in KDE before working on this bug, so my tests may not represent reality. I used the dbus interface to trigger a session save (org.kde.KSMServerInterface.saveCurrentSession), and then reloaded a session by running `okular --session xyz`, which I think is how KDE does it behind the scenes.
> 
> - Restore one or more documents in single window with tabs enabled.
> - Restore multiple windows, tabs enabled or disabled.
> - Restore session config describing multible tabs, even though tabs are disabled
> 
> 
> Thanks,
> 
> Jonathan Doman
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20150222/a8a01c91/attachment-0001.html>