Review Request 127004: WIP: OpenFileManagerWindowJob

[Emmanuel Pescosta]

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




src/widgets/openfilemanagerwindowjob.h (lines 79 - 102)
<https://git.reviewboard.kde.org/r/127004/#comment62869>

    Maybe add them as properties?



src/widgets/openfilemanagerwindowjob.h (lines 109 - 116)
<https://git.reviewboard.kde.org/r/127004/#comment62868>

    I would turn them into namespace functions, e.g. highlightInFileManager.
    
    (And I would only provide the url list version instead of both, because with C++11 it is as easy to use as the single url version, but this is only my personal opinion so feel free to ignore it ;)



src/widgets/openfilemanagerwindowjob.cpp (lines 95 - 97)
<https://git.reviewboard.kde.org/r/127004/#comment62871>

    I'm afraid that this code will become unreadable once the code of the other platforms will be added (do they have other fallbacks? or only KRun? what if dbus is available on those platforms? ....)
    
    This begs for the strategy pattern (instead of the unmaintainable ifdef crap) ;)
    
    You could inject the right strategy into the private class instance in the OpenFileManagerWindowJob::ctor and call a private class method here, which uses the injected strategy. In addition to that every strategy should call the fallback strategy which they think is best.
    
    KRun invocation can then also be implemented as a strategy, which is directly used when no dbus is available, or within the dbus strategy as fallback when the dbus call failed.


- Emmanuel Pescosta


On Feb. 7, 2016, 7:20 p.m., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127004/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2016, 7:20 p.m.)
> 
> 
> Review request for KDE Frameworks, Emmanuel Pescosta and Martin Gräßlin.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> This job uses the org.freedesktop.FileManager1 interface to highlight files within a certain folder, useful for eg. downloaded files or a "open containing folder" functionality.
> 
> 
> Diffs
> -----
> 
>   src/widgets/CMakeLists.txt 87dac50 
>   src/widgets/openfilemanagerwindowjob.h PRE-CREATION 
>   src/widgets/openfilemanagerwindowjob.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/127004/diff/
> 
> 
> Testing
> -------
> 
> With Dolphin's org.freedesktop.FileManager1 service installed, I got a file highlighted properly. Without it Dolphin opened the folder without highlighting (KRun fallback).
> 
> Not sure what to do with the startup id stuff
> 
> 
> Thanks,
> 
> Kai Uwe Broulik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160208/6d043d85/attachment-0001.html>