[Okular-devel] Review Request 110914: Tabbed interface

Sun Aug 25 16:22:04 UTC 2013

> On Aug. 18, 2013, 8:41 p.m., Albert Astals Cid wrote:
> > shell/main.cpp, line 112
> > <http://git.reviewboard.kde.org/r/110914/diff/3/?file=183075#file183075line112>
> >
> >     This looks like a separate feature than the tabs feature, maybe makes sense to split it to a different review to make it easier to review?
> 
> Jonathan Doman wrote:
>     By default, documents try to open as a new tab in an existing window. So without --new, the Detach Tab feature wouldn't work. Unless you want to change the default behavior, --new has to come in at the same time.
> 
> Albert Astals Cid wrote:
>     Oh, didn't see that you are spawning a whole new process. Why are you doing that? We can have multiple toplevel windows in a given process, no?
> 
> Jonathan Doman wrote:
>     I think it makes more sense to have each window in its own process, especially so that each window has its own DBus service. I want the command `okular file.pdf` to open file.pdf in the most recently activated window. Either we force all windows into one process and keep static state about the Shell activation times, or put each Shell into its own process and query activation times over DBus. It would be unnecessarily complicated to allow a mix, and I think the latter option is simpler to implement from the current state of things.

Well, we do have multiple windows in multiple processes now. Don't see why you need to change that to support tabs. If you want to propose this change, fine, but i can't see how it's necessary for the tabs feature.

> On Aug. 18, 2013, 8:41 p.m., Albert Astals Cid wrote:
> > part.cpp, line 838
> > <http://git.reviewboard.kde.org/r/110914/diff/3/?file=183074#file183074line838>
> >
> >     This looks a bit weird, you never initialize nor use m_dbusObjectName for anything other than for calling unregisterObject, is this something stale from old changes?
> 
> Jonathan Doman wrote:
>     I accidentally removed the initialization in the last update. I believe it's necessary so that closed tabs don't show up in DBus.
> 
> Albert Astals Cid wrote:
>     Are you sure about that? I expected the deletion of an object to cause it being unregistered from the bus, isn't that the case? It seems to work here in a simple test i made
> 
> Jonathan Doman wrote:
>     This may be another bug/feature with qdbusviewer (it crashes itself and okular if the parts aren't unregistered). How do you test dbus stuff? `dbus-send --dest=org.kde.okular-pid --print-reply / org.freedesktop.DBus.Introspectable.Introspect` shows closed parts if they aren't unregistered.

I'm just using qdbus command line tool with the current okular code.

Open two files in the same okular binary and i have
$ qdbus org.kde.okular-10275 
/okular
/okular2

Close one of the windows and

$ qdbus org.kde.okular-10275 
/okular

- Albert

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

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.
> 
> 
> 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
> 
> 
> This addresses bug 155515.
>     http://bugs.kde.org/show_bug.cgi?id=155515
> 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20130825/3905f719/attachment.html>