Both KeyUp and KeyDown listeners implemented, as described in bug.
BUG: 195801
No Linters Available |
No Unit Test Coverage |
Buildable 10096 | |
Build 10114: arc lint + arc unit |
This was created as a new revision due to --amend prior to arc diff. Not sure on how to update diff on previous one properly, previous is abandoned now.
To update it, you just run arc diff again while on your branch. No need to even do git add!
I've updated the diff with a couple questions I have (about UI mostly). Still WIP.
src/filewidgets/kurlnavigator.cpp | ||
---|---|---|
406 | QUrl(currentDirectory.resolved(levelUp)) for some reason results in missed 1st level directory, eg list for /home/neko would be: /home/neko / Is this a proper approach or should I do it somehow differently? | |
408 | Is QMenu a proper widget for dropdown list here? |
Thanks, this works great in Dolphin. However it does not work properly in Gwenview; the URL Navigator switches back to breadcrumbs mode every time the Up arrow is pressed. I've also got come coding and style suggestions:
src/filewidgets/CMakeLists.txt | ||
---|---|---|
15 | Unrelated whitespace change | |
42 | Unrelated whitespace change | |
src/filewidgets/kurlnavigator.cpp | ||
56 | Don't want this in production code | |
123 | Unrelated whitespace change | |
350 | This works in Dolphin, but causes the navigator to switch back to breadcrumbs mode in Gwenview. I think we need to instead fix whatever bug is causing the desire for thus ugly hack. :) | |
389 | Coding style: don't omit braces for single-line conditionals | |
404 | Don't want this in production code | |
425 | Unrelated whitespace change | |
486 | This feels like a hack | |
561 | Unrelated whitespace change | |
1301 | You could remove the true from these | |
src/filewidgets/kurlnavigator.h | ||
495 | Unrelated whitespace change | |
src/widgets/kurlcombobox.h | ||
113 ↗ | (On Diff #54815) | Too much indentation |
123 ↗ | (On Diff #54815) | In general we prefer using Enums rather than bools for function arguments. It makes the code much more readable. |
src/filewidgets/kurlnavigator.cpp | ||
---|---|---|
402 | Use toLocalFile() instead of path(), otherwise it breaks on Windows |
Thanks, I'll apply styling changes & will take a look at GWenView. Out of KDE Applications, what other consumers use KUrlNavigator? Is there an easy way to find out?
ngraham, in Dolphin, this commit is to blame for loosing editMode on changing URL: https://github.com/KDE/dolphin/commit/2af331b42c0514f4fdf848d0cc22f02717f7bec0
I don't think that this behavior has any logic in it to be honest; but it seems that it was explicitly implemented to work that way.
Yeah, it almost seems like that change was not really what was requested in the relevant bug report. Though it's kind of hard to tell.
Regardless, we don't work around apps' behavior in library components. So we should remove the workaround here and figure out how to handle Dolphin separately, either by reverting that change or fixing it so that it works with this new behavior in the URL navigator widget.
Yes, I was mostly pointing out that it seems to be out of scope of this revision to fix that problem :-)
https://bugs.kde.org/show_bug.cgi?id=157593#c4 this seems to be the most reasonable solution, if to keep the patch there. However, pressing "return" or otherwise changing URL is not same as UI element losing focus.
Also, given this: https://github.com/KDE/kio/blob/master/src/filewidgets/kurlnavigator.cpp#L336 I think it would actually be the best to revert that commit in dolphin.
This doesn't build:
/home/nate/kde/src/kio/src/widgets/kurlcombobox.cpp: In member function ‘void KUrlComboBox::setUrls(const QStringList&, KUrlComboBox::OverLoadResolving, KUrlComboBox::OldUrlPolicy)’: /home/nate/kde/src/kio/src/widgets/kurlcombobox.cpp:197:17: error: declaration of ‘QStringList urls’ shadows a parameter QStringList urls; ^~~~
src/filewidgets/kurlnavigator.cpp | ||
---|---|---|
58 | The existing list is not 100% sorted alphabetically, but let's assume alphabetical sorting for new entries, so this should go right below #include <QLabel> |
Whoops. I have a mix-up between two machines on which code is on, this compiles on one I've developed it on (no _urls/urls change), but not on one I've pushed from. I'll update this in several hours.
So, I'm really sorry for *very* long absence in this context. Updated diff.. Which works. I'll go over style and things tomorrow.
Thanks for resuming the work on this!
We've since moved to GitLab; could you open this up as a merge request at https://invent.kde.org/system/dolphin/-/merge_requests/? Thanks!