[Okular-devel] Review Request 110914: Tabbed interface

Wed Oct 16 21:30:24 UTC 2013

El Dimecres, 16 d'octubre de 2013, a les 23:19:51, Albert Astals Cid va 
escriure:
> 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.

Silly me, this was on the reviewboard, just could not find it ^_^

Sorry,
  Albert

> 
> 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
> 
> _______________________________________________
> Okular-devel mailing list
> Okular-devel at kde.org
> https://mail.kde.org/mailman/listinfo/okular-devel