[Okular-devel] Review Request 110914: Tabbed interface

[Albert Astals Cid]

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


Code looks good in general, some small comments


shell/shell.h
<https://git.reviewboard.kde.org/r/110914/#comment33093>

    Can we use ktabbar::currentIndex instead of keeping our own m_activeTab?



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33091>

    Any reason you went the KTabBar route instead the KTabWidget route? Seems KTabWidget provides all you need?



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33094>

    I'd like if you kept the OpenInNewTab/OpenInNewShell in a configuration setting so people were not forced to use tabs if they don't like them



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33092>

    indentation is off



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33086>

    Please use KStandardShortcut::tabNext and KStandardShortcut::tabPrev 



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33087>

    Probably you should add ret to the condition, no need to ask for the rest of parts if they want to be closed when we know we're not closing, no?



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33088>

    Who calls this?



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33089>

    I don't understand the comment



shell/shell.cpp
<https://git.reviewboard.kde.org/r/110914/#comment33090>

    This doesn't seem the best thing, given that okularpart knows the mimetype once it opens the file, i'd prefer a signal beign emmited instead another KMimeType call.


- Albert Astals Cid


On Dec. 16, 2013, 6:58 a.m., Jonathan Doman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/110914/
> -----------------------------------------------------------
> 
> (Updated Dec. 16, 2013, 6:58 a.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
> -----
> 
>   shell/shell.h c065c560fb4ddfcf181601cf35e9ca14581731f6 
>   shell/shell.cpp 1708501daaef817a1ce35fa5d96701a66ab66983 
> 
> 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/20131229/bd852b26/attachment-0001.html>