[Okular-devel] Review Request 110914: Tabbed interface

[Albert Astals Cid]

El Dimecres, 16 d'octubre de 2013, a les 18:36:52, Jonathan Doman va escriure:
> > 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).
> 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.

Can you please put this comment in reviewboard instead of directly to the 
list? It makes it easier to have stuff centralized in one place.

Thanks,
  Albert

> 
> 
> - Jonathan
> 
> 
> -----------------------------------------------------------
> 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