(depends on D11844)
Apply updated version of KLook patchset
use correct API to set the shortcut for openInKLook
use modern connect
Details
Details
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Branch
- klook
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Wow, it kind of works! Amazing!
Needs a lot of visual and usability polish work and bugfixing. But... this is awesome.
We probably want to make sure that Dolphin still works just fine without KLook since we don't want to make it a hard dep or regress anything in the time before it's polished up.
Comment Actions
As far as I know, KLook is an unmaintained playground repository, so any improvement should go in. Finding someone who is able to polish and release it would be even better, but let's start somewhere.
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1659 | Coding style nitpick: no whitespace after (. | |
| 1666 | Maybe range based for loop? | |
| 1671 | Coding style nitpick: if (!list.isEmpty()) {. Same down below. | |
| 1700 | #else Q_UNUSED(old) Q_UNUSED(now) #endif | |
| src/kitemviews/kfileitemlistwidget.cpp | ||
| 26 | Maybe, KIconLoader should be between KFormat and KLocalizedString? (to keep order) | |
| src/kitemviews/kfileitemlistwidget.h | ||
| 48 | virtual is redundant. | |
| src/kitemviews/kitemlistcontroller.cpp | ||
| 573 | Coding style nitpick: missing whitespace between ) and { | |
| src/kitemviews/kitemlistklooktoggle.cpp | ||
| 51 | Delete commented code? (or add a comment describing why it shouldn't be delted) | |
| src/kitemviews/kitemlistwidget.h | ||
| 35 | IMHO, it would be great to have the forward declared classes in order. | |
Comment Actions
At least in Diff Detail, Phab shows
Repository R318 Dolphin Branch klook
Edit: To clarify, there is D11844 for R604 KLook (File Previewer), while this Diff concerns the actual Dolphin integration.
Comment Actions
Ohhh.. my fault. But at least I got someone to actually look at it before the author decides to switch back to Mac :)
| src/kitemviews/kitemlistklooktoggle_p.h | ||
|---|---|---|
| 45 | Use nullptr. | |
Comment Actions
Oh my, I probably should have closed this. This was more intended to make testing https://phabricator.kde.org/D11844 possible, and the changes from there landed on KLook's qt5 branch. I deem the current Dolphin code not suited for upstream. KLook should probably changed so that it can be DBus activated and QProcess can be avoided, and applying this patch caused a few runtime warnings when using Dolphin if I recall correctly.
I don't have much time to work on KLook at the moment, and before doing any work on it, it should probably go to the VDG, because its current interface is less than stellar from a design and usability point of view.