Go up in folder hierachy when in "edit mode"
AbandonedPublic

Authored by krutovmikhail on Mar 24 2019, 7:43 PM.

Details

Reviewers
ngraham
Group Reviewers
Dolphin
Summary

Both KeyUp and KeyDown listeners implemented, as described in bug.

BUG: 195801

Diff Detail

Repository
R241 KIO
Branch
mkrutov/20190324/feat/kurlnavigator_keypresses_in_edit_mode
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10128
Build 10146: arc lint + arc unit
krutovmikhail created this revision.Mar 24 2019, 7:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 24 2019, 7:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
krutovmikhail requested review of this revision.Mar 24 2019, 7:43 PM
krutovmikhail retitled this revision from Initial adaptation of patch from Dolphin; keyUp -> goUp to WIP: Bug 195801 - go up in folder hierachy when in "edit mode".Mar 24 2019, 7:44 PM
krutovmikhail edited the summary of this revision. (Show Details)
krutovmikhail added reviewers: ngraham, Dolphin.

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!

  • initial take at dropdown list

I've updated the diff with a couple questions I have (about UI mostly). Still WIP.

src/filewidgets/kurlnavigator.cpp
404

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?

406

Is QMenu a proper widget for dropdown list here?

Updated - Working variant with proper widgets

krutovmikhail marked 2 inline comments as done.Mar 25 2019, 8:58 PM

After bit digging, found out a proper-er solution to both questions.

krutovmikhail retitled this revision from WIP: Bug 195801 - go up in folder hierachy when in "edit mode" to Bug 195801 - go up in folder hierachy when in "edit mode".Mar 25 2019, 8:59 PM
krutovmikhail edited the summary of this revision. (Show Details)
krutovmikhail edited the summary of this revision. (Show Details)

Updated - Working variant with proper widgets

Upated - Working variant with proper widgets - amending due to lost changes

ngraham retitled this revision from Bug 195801 - go up in folder hierachy when in "edit mode" to Go up in folder hierachy when in "edit mode".Mar 26 2019, 12:11 PM
ngraham edited the summary of this revision. (Show Details)
ngraham requested changes to this revision.Mar 26 2019, 12:21 PM

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
54

Don't want this in production code

124

Unrelated whitespace change

348

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. :)

387

Coding style: don't omit braces for single-line conditionals

402

Don't want this in production code

425

Unrelated whitespace change

474

This feels like a hack

561

Unrelated whitespace change

1287

You could remove the true from these

src/filewidgets/kurlnavigator.h
495

Unrelated whitespace change

src/widgets/kurlcombobox.h
113

Too much indentation

123

In general we prefer using Enums rather than bools for function arguments. It makes the code much more readable.

This revision now requires changes to proceed.Mar 26 2019, 12:21 PM
ngraham added inline comments.Mar 26 2019, 12:26 PM
src/filewidgets/kurlnavigator.cpp
400

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?

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?

Check this out: https://lxr.kde.org/ident?_i=KUrlNavigator&_remember=1

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.

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.

krutovmikhail marked 11 inline comments as done.
  • Update per phabricator comments: styling, boolean to enum, windows fix
krutovmikhail added a comment.EditedMar 26 2019, 9:17 PM

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.

  • Update per phabricator comments: styling, boolean to enum, windows fix

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
56

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.

This comment was removed by krutovmikhail.

What's the status of this?

Hi @ngraham,

Sorry for long time no update. I'll be getting back to this soon.

No problem, take your time. :)

meven added a comment.Jan 29 2020, 4:20 PM

Another great patch someone could commandeer, it seems...

  • fix build
  • Fixes for behaviour

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!

Great! Can you Abandon this?