Fix renamed file reclaims focus
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akrutzler created this revision.Sun, Jan 7, 3:44 PM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptSun, Jan 7, 3:44 PM
ngraham added inline comments.Sun, Jan 7, 4:10 PM
src/views/dolphinview.cpp
1360

an selection -> a selection

1391–1396

an selection -> a selection

ngraham accepted this revision.Sun, Jan 7, 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.Sun, Jan 7, 4:16 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

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

emateli added a comment.EditedMon, Jan 8, 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.Tue, Jan 9, 9:53 PM
akrutzler marked 2 inline comments as done.

Fix comments
Rebase

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.

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.Sun, Jan 14, 7:00 PM
This revision now requires changes to proceed.Sun, Jan 14, 7:00 PM

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

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

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.Fri, Jan 19, 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.Fri, Jan 19, 11:00 PM

No problem, thanks for your review! :)

@elvisangelaccio, are you good with this now?

elvisangelaccio requested changes to this revision.Sat, Jan 20, 7:30 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.

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.Sat, Jan 20, 7:30 PM

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.Sun, Jan 21, 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.Sun, Jan 21, 3:12 PM

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

This revision was automatically updated to reflect the committed changes.