#314258 - Duplicate tab settings when duplicating tabs

Authored by palant on Feb 2 2017, 8:58 PM.


Group Reviewers

Whenever a new tab is created with this change we'll try to copy configuration from an existing tab. I'm not really familiar with the code but this seems to be the right way to do it.

Test Plan

When duplicating tabs, various tab settings should be duplicated as well:

  • Sort column and order
  • Zoom factor
  • View type (brief vs. detailed)
  • View filter (all files vs. custom)
  • "Show Previews" setting

There are also some ListPanel properties being duplicated but I don't know how these are set.

Tab duplication should not create unnecessary sections in the config file.

Diff Detail

R167 Krusader
Lint Skipped
Unit Tests Skipped
palant updated this revision to Diff 10872.Feb 2 2017, 8:58 PM
palant retitled this revision from to #314258 - Duplicate tab settings when duplicating tabs.
palant updated this object.
palant edited the test plan for this revision. (Show Details)
palant added a reviewer: Krusader.
palant set the repository for this revision to R167 Krusader.
palant added a project: Krusader.
abika added a subscriber: abika.Feb 4 2017, 2:13 PM

Hmm, works fine but using a temporary config group seems a bit hackish.

I'd also say I don't like it that much. And maybe we don't want to duplicate every setting but just some of it - e.g. the one regarding KrView? My proposal is to create alternative KrView::restoreSettings method which would take another KrView reference as a parameter and load the settings directly from this given view. Any other suggestions? :)

palant added a comment.Feb 4 2017, 7:52 PM

Yes, I used a temporary config group because the code in PanelManager::slotRecreatePanels() does it like that - this might not be the cleanest approach however. I will add a ListPanel::duplicateSettings(ListPanel*) method instead that restores a different set than ListPanel::restoreSettings(KConfigGroup). The danger here is however that these two methods get out of sync as new settings are added - some new settings will be restored by the latter but not the former even where both would make sense.

palant updated this revision to Diff 10952.Feb 6 2017, 8:47 AM
palant edited the test plan for this revision. (Show Details)

I now implemented "proper" settings duplication as suggested. Frankly, I'm not convinced that this approach is better. It introduces quite a bit of code which is almost the same as what was there before but not quite. Also, is there really any setting where it makes sense to restore it on startup but not to duplicate when you duplicate the tab? The only one I left out here is the tab history but even that one should arguably better be duplicated.

As to what this approach does better: there is definitely less overhead. But is that worth it, with an action that is triggered by the user? It's not like the user is going to notice the difference between 5ms and 20ms. Note: these numbers are random, I didn't really measure the performance - I didn't notice a delay with either approach.

abika added a comment.Feb 9 2017, 8:44 PM

Ok, I see now that this is also not a good solution. The settings are going down to the PanelPopup and KrInterviews. All this should be copied. Sorry I didn't got this earlier.

So the previous solution wasn't pretty but is good enough. Maybe add a comment that panel settings are duplicated by saving/restoring to a temporary group.
And there is still a minor flaw: the URL of the current panel is set two times to the new one.

In case we need to differentiate between real save/restore and duplicate settings a boolean flag should do it.

And a performance difference of 20ms does not matter at all. What is important is that the code is good.

Alex have good points as well as You Wladimir about the simplicity of the previous code. I also agree now with the previous approach and sorry about the previous suggestion then.

One thing I've noticed now, testing again both approaches. There seems to be a corner case: when duplicating locked tab, 2 tabs are actually created.

palant updated this revision to Diff 11179.Feb 10 2017, 3:17 PM

I reverted to the original approach and applied suggested changes. As far as locked tabs are concerned, everything seems to work correctly for me. Could it be that the issue there was being caused by the extra ListPanel::start() call?

abika accepted this revision.Feb 10 2017, 4:16 PM
abika added a reviewer: abika.

Could it be that the issue there was being caused by the extra ListPanel::start() call?

Quick test says: yes!

Thanks for the patch!

[Can you land/push the diff yourself? I don't know about access rights here.]

This revision is now accepted and ready to land.Feb 10 2017, 4:16 PM
martinkostolny accepted this revision.Feb 10 2017, 4:29 PM
martinkostolny added a reviewer: martinkostolny.

Yes, that must have been it. It now works nicely, thanks for the patch! :)

asensi accepted this revision.Feb 11 2017, 2:23 PM
asensi added a reviewer: asensi.
asensi added a subscriber: asensi.

It can be compiled and executed using Kubuntu 16.04 or 16.10, and everything seems to work correctly :-), thanks Wladimir!

abika closed this revision.Feb 13 2017, 8:54 PM

Merged with master