----------------------------------------------------------- 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
