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.
Details
Details
- Reviewers
asensi martinkostolny abika - Group Reviewers
Krusader
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
Diff Detail
- Repository
- R167 Krusader
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
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? :)
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
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.]
Comment Actions
It can be compiled and executed using Kubuntu 16.04 or 16.10, and everything seems to work correctly :-), thanks Wladimir!