Review Request 110487: Bug 196035 - middle clicking on archive files in dolphin does not open them in a new tab

[Emmanuel Pescosta]

> On May 23, 2013, 11:40 a.m., Frank Reininghaus wrote:
> > Thanks for the patch! The approach looks good, but I think that a function DolphinView::itemCanBeOpenedAsFolder(const KFileItem& item, KUrl& url) which has side effects on the argument 'url' is not a perfect solution. I see though that there is no obvious more elegant alternative :-(
> > 
> > I'll think about this... Maybe we could make DolphinView::itemCanBeOpenedAsFolder(KFileItem) only check if opening the item is possible and adjust the 'protocol' of the URL somewhere else (if necessary), like in DolphinView's constructor and setUrl()? One could then also move the 'GeneralSettings::browseThroughArchives()' check out of that function - I see no reason why middle-clicking an archive should not open it even if that setting is disabled. What do you think?
> 
> Emmanuel Pescosta wrote:
>     > DolphinView::itemCanBeOpenedAsFolder(const KFileItem& item, KUrl& url) which has side effects on the argument 'url' is not a perfect solution
>     Yes I agree with you. 
>     My first idea was to return a empty url if the item could not be opened as a folder and return the adjusted url if the item could be opened as a folder, but with this solution we need a lot of url.isEmpty() checks, which is also far from perfect I think ;)
>     
>     Your solution sounds great :) Moving the protocol adjustment code to DolphinView's setUrl() function makes sense.
>     
>     But we need a new DolphinView::setUrl(const KFileItem& item) member function here, because we need to know the mimetype of the item to adjust the url (Look at DolphinView::itemCanBeOpenedAsFolder).
> 
> Frank Reininghaus wrote:
>     Ah yes, you're right, of course. Well, if we have to add a new overloaded setUrl(KFileItem) member, then my proposal is not really elegant any more...
>     
>     I think your 'first idea' to use a function that returns the URL to be used for opening the KFileItem as a folder (or an empty URL if there is no way to open it as a folder) sounds better then. As far as I can see, there are only two places where an isEmpty() check would be needed, right? From my point of view, this looks better than having this side effect in the function returning a bool. What do you think?

5 places ;) But we can remove some isDir() checks.

Ok I will modify the patch. How should I name this function? Maybe "openItemAsFolderUrl(const KFileItem& item)"?


- Emmanuel


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


On May 17, 2013, 9:17 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110487/
> -----------------------------------------------------------
> 
> (Updated May 17, 2013, 9:17 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> Description
> -------
> 
> When 'browse through archives' is enabled, open archive files
> like folders on middle clicking, context menu -> new tab action
> and context menu -> new window action.
> 
> 
> This addresses bug 196035.
>     http://bugs.kde.org/show_bug.cgi?id=196035
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphincontextmenu.cpp af8b613 
>   dolphin/src/dolphinmainwindow.cpp 347489d 
>   dolphin/src/dolphinviewcontainer.cpp 0e413bc 
>   dolphin/src/views/dolphinview.h e50dffc 
>   dolphin/src/views/dolphinview.cpp abf572f 
> 
> Diff: http://git.reviewboard.kde.org/r/110487/diff/
> 
> 
> Testing
> -------
> 
> Middle clicking or context menu -> new tab action opens the archive in a new tab, if 'browse through archives' is active.
> 
> Context menu -> new window action opens the archive in a new window, if 'browse through archives' is active.
> 
> Works for me.
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130527/cf4c81d0/attachment.htm>