Differential D9711

Fix renamed file reclaims focus
ClosedPublic
Actions

Authored by akrutzler on Jan 7 2018, 3:44 PM.

Details

Reviewers
ngraham
michaelh
elvisangelaccio
Group Reviewers
Dolphin
Commits
R318:bba44ae907b4: Fix renamed file reclaims focus
Summary

After renaming a file and then selecting another file immediately the just selected file stays selected.

BUG: 388555

Test Plan

Steps to reproduce:
$ mkdir /tmp/test
$ cd /tmp/test
$ touch a.tmp b.tmp
$ dolphin /tmp/test

In dolphin:

  • select a.tmp
  • <F2>
  • type aaa
  • select b.tmp immediately

Expected result:

  • a.tmp renamed to aaa.tmp
  • b.tmp stays selected, aaa.tmp stays unselected

Diff Detail

Repository
R318 Dolphin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
akrutzler created this revision.Jan 7 2018, 3:44 PM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptJan 7 2018, 3:44 PM
akrutzler requested review of this revision.Jan 7 2018, 3:44 PM
ngraham added inline comments.Jan 7 2018, 4:10 PM
src/views/dolphinview.cpp
1360

an selection -> a selection

1391

an selection -> a selection

ngraham accepted this revision.Jan 7 2018, 4:16 PM

Thanks for the patch! It fixes the issue and doesn't regress the original feature.

This revision is now accepted and ready to land.Jan 7 2018, 4:16 PM
elvisangelaccio added a subscriber: elvisangelaccio.Jan 7 2018, 5:25 PM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1362

I'm not 100% convinced. Why does the model return a wrong index? If I select b.tmp, here I'd expect the index for b.tmp, not the one for a.tmp

elvisangelaccio added a subscriber: emateli.Jan 7 2018, 8:59 PM

@akrutzler You are right that this is a regression after 478f404b8a.
@emateli Can you have a look?

emateli added a comment.EditedJan 8 2018, 1:38 PM

Unfortunately I do not have as much time as I would like to investigate this, however I suspect it may have to do with slotRenamingFinished and forceUrlsSelection. As in the slot is emitted after the files(s) have finished renaming but at that point the user has already changed the selection. If that's the case it may make a bit more sense to handle this inside the forceUrlSelection method instead.

akrutzler updated this revision to Diff 25047.Jan 9 2018, 9:53 PM
akrutzler marked 2 inline comments as done.

Fix comments
Rebase

akrutzler added a comment.Jan 9 2018, 9:53 PM
In D9711#187679, @emateli wrote:

Unfortunately I do not have as much time as I would like to investigate this, however I suspect it may have to do with slotRenamingFinished and forceUrlsSelection. As in the slot is emitted after the files(s) have finished renaming but at that point the user has already changed the selection. If that's the case it may make a bit more sense to handle this inside the forceUrlSelection method instead.

Hm I could not find a way to solve this inside the forceUrlSelection method. But maybe I am missing something.

src/views/dolphinview.cpp
1362

m_currentItemUrl is not set when a file is selected. I tried to set it when the selection changes, but that does not work. Therefore, at this point, m_currentItemUrl stores the name of the file you just renamed.

michaelh added a comment.Jan 14 2018, 11:03 AM

Does not work for me.

For testing:

select a.tmp
<F2>
type aaa

Do not hit <Enter>
Select b.tmp with mouse - No need for speed :-)

src/views/dolphinview.cpp
1597

Observation:
With this line commented out, it sometimes works.
When finishing rename with 'Enter' renamed file looses focus.

michaelh requested changes to this revision.Jan 14 2018, 7:00 PM
This revision now requires changes to proceed.Jan 14 2018, 7:00 PM
akrutzler added a comment.Jan 19 2018, 10:14 AM

When I do this, a.tmp is renamed to aaa.tmp and b.tmp remains selected. For me, that's the result one would expect.

src/views/dolphinview.cpp
1597

Without this line, D6312 doesn't work anymore :)

michaelh added a comment.Jan 19 2018, 2:26 PM

Maybe our dolphins differ. With fresh compile from origin/Applications/17.12 this is unexpected

rename.mkv50 KBDownload

akrutzler added a comment.Jan 19 2018, 5:51 PM
In D9711#193368, @michaelh wrote:

Maybe our dolphins differ. With fresh compile from origin/Applications/17.12 this is unexpected

I usually test against the latest master. Did you apply my patch after checking out origin/Applications/17.12? The easiest way to do this is using arc, a command-line interface to Phabricator. To apply this particular patch use arc patch D9711.
@ngraham are you able to reproduce this behavior?

michaelh accepted this revision.Jan 19 2018, 11:00 PM

Just rechecked it. It does indeed work as expected. I'm very sorry, I must have muddled something up.

This revision is now accepted and ready to land.Jan 19 2018, 11:00 PM
akrutzler added a comment.Jan 19 2018, 11:05 PM

No problem, thanks for your review! :)

ngraham added a comment.Jan 20 2018, 2:39 AM

@elvisangelaccio, are you good with this now?

elvisangelaccio requested changes to this revision.Jan 20 2018, 7:30 PM
In D9711#188459, @akrutzler wrote:
In D9711#187679, @emateli wrote:

Unfortunately I do not have as much time as I would like to investigate this, however I suspect it may have to do with slotRenamingFinished and forceUrlsSelection. As in the slot is emitted after the files(s) have finished renaming but at that point the user has already changed the selection. If that's the case it may make a bit more sense to handle this inside the forceUrlSelection method instead.

Hm I could not find a way to solve this inside the forceUrlSelection method. But maybe I am missing something.

I had a look and this bug is caused by the forceUrlsSelection() call in slotRoleEditingFinished() (added by 478f404b8a).
The problem is that we are marking the item being renamed as the m_currentItemUrl, but that is not necessarily correct as this bug shows.

@emateli is right that forceUrlsSelection() is a better place to put this check in. Adding

if (m_container->controller()->selectionManager()->hasSelection()) {
    return;
}

at the beginning of the method fixes the bug for me.

This revision now requires changes to proceed.Jan 20 2018, 7:30 PM
akrutzler added a comment.Jan 21 2018, 2:09 PM
In D9711#193793, @elvisangelaccio wrote:

I had a look and this bug is caused by the forceUrlsSelection() call in slotRoleEditingFinished() (added by 478f404b8a).
The problem is that we are marking the item being renamed as the m_currentItemUrl, but that is not necessarily correct as this bug shows.

@emateli is right that forceUrlsSelection() is a better place to put this check in. Adding

if (m_container->controller()->selectionManager()->hasSelection()) {
    return;
}

at the beginning of the method fixes the bug for me.

With that, D6312 doesn't work anymore. Because when calling forceUrlsSelection() in slotRoleEditingFinished(), there is always a selection.

elvisangelaccio accepted this revision.Jan 21 2018, 3:12 PM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1597

Ah right, of course.

Well, let's go with this patch then. But please push to master only, I'm not confident that we won't break something else :/

This revision is now accepted and ready to land.Jan 21 2018, 3:12 PM
ngraham added a comment.Jan 21 2018, 8:40 PM

I don't think Andreas has commit access, so I'll push to master.

Closed by commit R318:bba44ae907b4: Fix renamed file reclaims focus (authored by akrutzler, committed by ngraham). · Explain WhyJan 21 2018, 8:43 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/views/dolphinview.cpp (59 lines)

Diff 24884

View Options

src/views/dolphinview.cpp

Loading...