[Okular-devel] Review Request 110914: Tabbed interface

Sat Oct 19 18:52:21 UTC 2013

> On Oct. 1, 2013, 6:57 p.m., Fabio D'Urso wrote:
> > Hi! :)
> > I have neither tested nor read the whole patch in depth, I've only had a look at your description and made a few tests.
> > 
> > We had a discussion at the okular BOF this summer, and we decided that it's better to avoid having multiple open copies of the same document, because this creates consistency issues if you are annotating (while saving, the two instances will overwrite each other's changes). In the light of this, I'm not sure "Duplicate tab" makes sense the way it is now. In my opinion, a better approach would be to tie multiple PageViews to the same Document. However, as the Document class currently assumes that there is only one PageView, this requires significant changes, so we had better leave this feature for a different patch.
> > 
> > I see a lighter issue with the way "Detach tab" works: if you create a new process you will 1) loose all cached pixmaps, 2) cause the Part::queryClose() message to show up if there are unsaved annotations. (If you want to reproduce this issue annotate a PDF, click "save as", open the file you saved, detach its tab).
> > 
> > Both "Duplicate tab" and "Detach tab" fail at handling input from - (standard input)
> >   cat somePdfFile.pdf | okular -
> > because, by definition, you can't read it twice.
> > 
> > About the issue with moving menu items, I see that all you need to do is to add the confirm_tabs_close checkbox, right? Maybe you can just add it to the configuration dialog instead or drop it at all (for example the warnings from document.cpp warnLimitedAnnotSupport() cannot be reactivated).
> 
> Jonathan Doman wrote:
>     So should I remove the duplicate tab feature? It's possible in the master code to open the same document in two windows, and it would still be possible in this code to manually open the same document to get it in another tab. Is that going to be blocked in the future? If not, I don't see how duplication causes any additional problems.
>     
>     Do you agree with the logic to open new documents in the MRU existing window? No one has given their opinion on this yet. If not, then it's easy to keep documents in the same process and I'll do that.
>     
>     I'll take a look at the other stuff and upload a new diff when I get some time.

To be honest, you should aim simple, i.e. implement tabs, don't try to do anything else like the process per window, or attaching to the MRU or anything. Because they don't really belong into a "tabbed interface" feature. I'm not saying they are not interesting features and i'm all for discussing if we want them or not, but them being in this Merge Request just makes it harder for it to be merged, because it increases the number of changes and thus increases the number of times people can disagree.

So please try to give us the smallest possible diff that adds tabs to okular. And then once we merge that, we can work on that.

Thanks again and sorry for the delay :-)

P.S: We have 10 days until 4.12 feature freeze, i'll try to be as responsive as i can reviewing/answering comments to get it in for 4.12.

- Albert

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110914/#review41071
-----------------------------------------------------------

On Aug. 23, 2013, 8:06 p.m., Jonathan Doman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110914/
> -----------------------------------------------------------
> 
> (Updated Aug. 23, 2013, 8:06 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 155515
>     http://bugs.kde.org/show_bug.cgi?id=155515
> 
> 
> Repository: okular
> 
> 
> Description
> -------
> 
> This patch adds support for a tabbed interface (multiple documents in one window). The core work just adds a tab bar that switches between multiple embedded okularparts, but there are many other considerations:
>  - Tab context menu allows for duplicating or detaching (detached tabs start in new okular process)
>  - `okular file.pdf` will open file in existing window if possible, unless --new flag is used. It also selects the most recently raised/activated window to use. This mirrors behavior I expect from browsers and other tabbed interfaces.
>  - Warns when closing window with multiple tabs
>  - No warning is given when opening an already open file. This is the behavior I strongly prefer (and observe in other programs), but will change if there is consensus otherwise.
> 
> When selecting different tools in one part, the tool selection propagates to all parts, but the GUI does not reflect that. This bug is present in other programs (e.g. multiple okularparts in Konqueror), so I made no attempt to diagnose or fix.
> 
> One menu item was added for the multiple tab warning option. When testing this, I noticed that items in the Settings menu seem to move around when switching tabs, and I cannot diagnose or fix this. It seems to be related to XMLGUI bug #64754. 
> 
> My development branch is also hosted at https://github.com/jrmrjnck/okular-tabbed
> 
> 
> Diffs
> -----
> 
>   part.h 4b3aafdb637080ae81eb0e45742f53a34738984d 
>   part.cpp 71c3d0f5d908969ffcf18aba327297ccd2e1c8e1 
>   shell/main.cpp e0ca587ba167c4020d5af5bd33a2dc1b4923cac4 
>   shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 
>   shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 
>   shell/shell.rc 93fbc417588312792bab39b693c65e5d414c87c6 
> 
> Diff: http://git.reviewboard.kde.org/r/110914/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jonathan Doman
> 
>

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