Review Request 127004: WIP: OpenFileManagerWindowJob

[David Faure]

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




src/widgets/openfilemanagerwindowjob.h (line 44)
<https://git.reviewboard.kde.org/r/127004/#comment65284>

    "On Linux, this job will..."



src/widgets/openfilemanagerwindowjob.h (line 50)
<https://git.reviewboard.kde.org/r/127004/#comment65285>

    This says "Also note" ... but it really says the same as the previous paragraph, just in more words. Maybe swap these two paragraphs? I.e. "If you just want to open a folder" is the conclusion.



src/widgets/openfilemanagerwindowjob.h (line 114)
<https://git.reviewboard.kde.org/r/127004/#comment65286>

    @since 5.24 here as well



src/widgets/openfilemanagerwindowjob.cpp (line 101)
<https://git.reviewboard.kde.org/r/127004/#comment65288>

    Useless conversion followed by useless test, you already test the url-list for emptiness at the beginning of the method.



src/widgets/openfilemanagerwindowjob.cpp (line 123)
<https://git.reviewboard.kde.org/r/127004/#comment65290>

    add link to the spec for this, in a comment?



src/widgets/openfilemanagerwindowjob.cpp (line 138)
<https://git.reviewboard.kde.org/r/127004/#comment65291>

    simpler to just put it on the stack



src/widgets/openfilemanagerwindowjob.cpp (line 151)
<https://git.reviewboard.kde.org/r/127004/#comment65294>

    KJobWidgets::window(q)



src/widgets/openfilemanagerwindowjob.cpp (line 155)
<https://git.reviewboard.kde.org/r/127004/#comment65292>

    This TODO looks like a copy-paste error, there is no todo here.



src/widgets/openfilemanagerwindowjob.cpp (line 156)
<https://git.reviewboard.kde.org/r/127004/#comment65289>

    missing return statement here, otherwise you're emitting result twice.



src/widgets/openfilemanagerwindowjob_p.h (line 50)
<https://git.reviewboard.kde.org/r/127004/#comment65293>

    Strange naming, usually 'q' used in FooPrivate to point to Foo. Rename to job?


- David Faure


On June 16, 2016, 7:35 p.m., Kai Uwe Broulik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127004/
> -----------------------------------------------------------
> 
> (Updated June 16, 2016, 7:35 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 fee199f 
>   src/widgets/openfilemanagerwindowjob.h PRE-CREATION 
>   src/widgets/openfilemanagerwindowjob.cpp PRE-CREATION 
>   src/widgets/openfilemanagerwindowjob_p.h 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/20160617/2680d42b/attachment.html>