| ngraham | |
| rkflx | |
| elvisangelaccio |
| Dolphin |
Scrolling during inline renaming accepts the renaming now, like if one would hit Return for example. I chose this approach because it seems the easiest way to fix this.
This also fixes the “possible” Ui glitch where the renaming KTextField doesn’t move along with the list item. Possible glitch, because I don’t know if this is intentional, but for me it looks broken.
BUG: 378786
Fixes T7443
| No Linters Available |
| No Unit Test Coverage |
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 61 | Why is this new function necessary? | |
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 61 | Actually it's not necessary. Just my personal preference since KIO::encodeFileName(toPlainText() is used 3 times (2 in kitemlistroleeditor.cpp and 1 in kstandarditemlistwidget.cpp). But I can also remove this function. | |
Wow, can't wait to test this! (Busy currently, give me a day or two).
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 61 | In general I'd just follow the same style as the surrounding code. I don't think there is a rule specifying a point at which you are supposed to refactor into a function. I'd probably just keep what you have done right now and wait whether Dolphin's maintainers like it or not (they should give the final green light anyway). | |
I've tested this and it works great. As soon as the unnecessary new function is removed, I'll approve it and will wait for @rkflx to review as well.
Had a look, works great, solves the bug and I cannot break it. Code seems fine. I still don't understand what is the actual problem in the bug, but at least it is not triggered anymore and the floating text input field is gone. Now shows same behaviour as in Windows Explorer, which is fine.
@akrutzler Please add Fixes T7443 to the summary :) Also, the title should not sound like a bug, instead reword to either describe the new behaviour, or add "Fix <the problem>".
@elvisangelaccio Is this approvable from Dolphin's perspective?
Note for the committer: This is currently branched from master, but should probably go to Applications/17.12.
Thanks for working on this.
Can you try to use the event filter in KItemListRoleEditor instead? It should be enough to also check for the QEvent::Wheel event.
It seems to work for me and the patch becomes much simpler.
The actual problem behind this is an "miscommunication" between View and Model. If you want to rename something, you tell the KItemListView to change the text-role of one of its items by calling KItemListView::editRole(int index, const QByteArray& role). index defines which item to change. But this index is not persistent! Once you scroll through the list, that index may change (because of some performance optimizations in KItemListView::doLayout()i guess). Because of this, the index of an Item in the View differs from the index in the Model(where necessary data like the URL is stored). But by design they must stay the same!
I tried to catch the QEvent::Wheel event in KStandardItemListWidget but of course that doesn't work because KItemListRoleEditor has focus. If that works, this patch should be very simple thanks :) Maybe I'll make it until tonight.
@akrutzler Thanks for the analysis, sounds like you are onto another fix :) If you are interested (if not, I can also recommend a few Gwenview bugs): https://bugs.kde.org/show_bug.cgi?id=334533#c7
This patch is pretty simple now. The behaviour is slightly different. Now the renaming gets accepted once you turn the mouse wheel, even if
there is actually nothing to scroll. But i like it that way too :)
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | Not all users will be using a mouse with a wheel. Can we use QEvent::Scroll instead, so this continues to work with a laptop touchpad or touchscreen? | |
334533 seems to be a tricky one, but definitely related to this Model/View bug, lets see what i can do. I am not very familiar with gwenview as a user, but yes i can have a look at it :)
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | I tested this with my touchpad too and it works pretty well. But since i have no touchscreen available right now i cannot test this case. But then i have to catch QEvent::Wheel and QEvent::Scroll right? Because i don't get any scroll events when using mouse/touchpad. | |
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | If what you have works with a touchpad already, then that's fine. It's not like Dolphin is very well optimized for touchscreen use anyway. | |
Sorry to say, but now it breaks for me: Try scrolling with the mouse wheel over the scrollbar (can easily happen if you are trying to move the mouse "out of the way" or when using the context menu for Rename). We do not get the scroll event anymore, the line edit is floating again and the rename target becomes unpredictable.
I tried qDebug() << event, but at this point there's no event at all to observe. Either we go back to the previous version of the patch, or we find another elegant solution…
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | Touchpad support was the first thing I looked up after Elvis mentioned this :). Apparently "Wheel events are generated for both mouse wheels and trackpad scroll gestures." Source As for testing touchscreen support, maybe there's some trickery you can setup in X11 to emulate touch events with a regular mouse? | |
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | Can you try event->type() == QEvent::Expose | |
| src/kitemviews/private/kitemlistroleeditor.cpp | ||
|---|---|---|
| 64 | In my former approach, the renaming was accepted once the View "scrolls" its content. It was completely independent of what the scrolling actually triggered since i was not catching any event. This could be either by mouse/touchpad or even programmatically by calling KItemListView::scrollToItem for example. IMO that should work for touchscreens too. I tried to catch QEvent::Expose in KItemListRoleEditor::eventFilter and KItemListRoleEditor::event but i don't even get this event. :\ | |
Let's not get too sidetracked on the touchscreen topic. I'm not even sure Dolphin supports scrolling the view on a touchscreen without resorting to the scrollbar.
Also not sure if fiddling with the painting code/visibility (i.e. what is related to expose events) helps us here.
As I said, there are two approaches:
The problem with catching the wheel event in a different place is that i still need a "connection" between this new place and the KItemListRoleEditor (because the new name is stored in there). So i don't know if that makes it simpler. It would be easier/prettier if we cancle the renaming instead of accepting it, because in that case i don't care about KItemListRoleEditor. But accepting it is the right way to go :)
Unless I find a better solution for it, I'll just go back to my previous approach.
Reverting to previous patch, beacause reacting to KItemListView::scrollOffsetChanged-signal is independent of what/who actual causes the
scrolling.
| src/kitemviews/kstandarditemlistwidget.h | ||
|---|---|---|
| 189 | Maybe let's call it emitRoleEditingFinished() ? (which is what eventually happens) finishRoleEditing() could also work... | |
Any reason this has been committed to master only?
(Interestingly, I just noticed what Andreas fixed here was still working in a KDE4 based Dolphin…)
I think this bug was introduced in Dolphin 2.0(4.8), where he got his new view engine six years ago. Somewhere around f23e9496f303995557b744c03178f5dbd9b35016. Just a guess because i am not able to build this version.
On a stock Ubuntu 16.04 Dolphin (15.12.3, Qt 5.5.1, KF 5.18) it works, however when compiling 15.12.3 on a recent system, it does not work. Weird. So it is probably more complicated or somewhere outside of Dolphin.
However (just checked), 334533 is already broken in the old Ubuntu as well as KDE4 (post-2.0, i.e. 15.04), so probably not worth bisecting this further…
Cherry-picked on the stable branch.
@ngraham Next time please push bugfixes to the Applications/xx.yy branch, then merge on master.