[Okular-devel] Review Request 110914: Tabbed interface

[Jonathan Doman]

> On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 181
> > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line181>
> >
> >     This check seems a bit weird, i don't even think you need it, but if you want one check there shouldn't you be doing something like
> >     activeTab < m_tabs.size?
> >     
> >     But this can't happen, no?

Yes, you are right. I was specifically trying to change the code as little as possible, and there was previously a check for m_part which was also weird and unnecessary.


> On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 206
> > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line206>
> >
> >     should this be m_tabs[activeTab] instead of m_tabs[0]?

Maybe. In that section of code it is guaranteed that activeTab==0, since there will never be an empty part when there are other tabs open. I was going to put an assertion in there to make that clear, but I noticed there are no assertions anywhere in the code and thought it might be some kind of policy.


> On Jan. 12, 2014, 9:57 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 256
> > <https://git.reviewboard.kde.org/r/110914/diff/9/?file=233820#file233820line256>
> >
> >     space here

Are you sure? Most config keys (in this program and others) are camel cased.


- Jonathan


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


On Jan. 11, 2014, 10:32 p.m., Jonathan Doman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/110914/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2014, 10:32 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 05a0a62265c7e7ed79d719a3f648850f8ef642e5 
>   shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 
>   shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 
>   shell/shell.rc 93fbc417588312792bab39b693c65e5d414c87c6 
> 
> Diff: https://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/20140112/e69c9a67/attachment.html>