[Okular-devel] Review Request 110914: Tabbed interface

Thu Jan 9 00:16:45 UTC 2014

> On Dec. 29, 2013, 7:44 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 203
> > <https://git.reviewboard.kde.org/r/110914/diff/6/?file=225169#file225169line203>
> >
> >     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
> 
> Jonathan Doman wrote:
>     Where should the interface for this setting be?
> 
> Albert Astals Cid wrote:
>     Settings -> Configure -> General -> Program features looks good to me. Waht you think?
> 
> Jonathan Doman wrote:
>     That configure dialog is for the okular part, while the setting is only applicable to the shell (and not other part users like konqueror). Seems like it should just be a checkbox in the Settings menu since there are no other shell options.
> 
> Albert Astals Cid wrote:
>     You're right, but on the other hand the shell/part split is something technical, the user should not care much.
>     You're also right in which we should not show the option in other shells like konqueror.
>     
>     Still I don't think a settings checkbox menu is the best option. My idea of the ideal scenario would be the part providing an interface as the ones in interfaces/ where you can add your own configuration stuff to the dialog.
>     
>     But for the sake of not having this review last forever, ok, let's do the checkbox in the settings menu and I'll see if I can implement my idea.

I agree with everything you said. For now, I have added a checkbox next to the FullScreen setting. It opens documents in a new tab by default and can be unchecked to use new windows instead.

> On Dec. 29, 2013, 7:44 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 551
> > <https://git.reviewboard.kde.org/r/110914/diff/6/?file=225169#file225169line551>
> >
> >     I don't understand the comment
> 
> Jonathan Doman wrote:
>     Using sender() is generally considered a Bad Practice, but it's simpler than using a signal mapper (greatly so if we will ever allow the tabs to be moved/reordered). The comment is there for others who may be inclined to copy/paste the code.
> 
> Albert Astals Cid wrote:
>     Personally i don't feel we need that warning, but ok.

Comment was removed when refactoring

> On Dec. 29, 2013, 7:44 p.m., Albert Astals Cid wrote:
> > shell/shell.cpp, line 589
> > <https://git.reviewboard.kde.org/r/110914/diff/6/?file=225169#file225169line589>
> >
> >     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.
> 
> Jonathan Doman wrote:
>     Is KMimeType expensive? Are you suggesting that I add a new signal to Okular::Part?
> 
> Albert Astals Cid wrote:
>     It is not expensive, but we're doing quite a few "tricky" things in the mimetype determination side besides, and that would probably like when you open stuff form the stdin piping, so not doing the same twice seems like a good idea. Yes, i'm suggesting a new signal or add a paramater to an existing one if you find something suitable.

Okular::Part now emits a signal after calculating the mimetype in openFile(). This is used by the Shell to set the tab icon.

- Jonathan

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

On Jan. 9, 2014, 12:15 a.m., Jonathan Doman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/110914/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2014, 12:15 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
> -----
> 
>   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/20140109/b42af8c5/attachment.html>