Improve profile management
ClosedPublic

Authored by ahmadsamir on Dec 9 2017, 5:31 PM.

Details

Summary

BUG: 358084

Change the edit profile dialog so that the profile can't be saved if:

  • The profile config file is in a read-only dir (e.g. /usr/share/konsole)
  • The name is empty or if the name matches the name of another already

existing profile.

If a profile gets renamed:

Change the Manage Profiles dialog:

  • The profile can only be renamed from the edit profile dialog so that we can check if the profile can be saved (from EditProfileDialog)
  • Double clicking the profile name opens the edit profile dialog
Test Plan

Diff Detail

Repository
R319 Konsole
Lint
Lint Skipped
Unit
Unit Tests Skipped
ahmadsamir created this revision.Dec 9 2017, 5:31 PM
Restricted Application added a project: Konsole. · View Herald TranscriptDec 9 2017, 5:31 PM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
ahmadsamir requested review of this revision.Dec 9 2017, 5:31 PM
ngraham added a subscriber: ngraham.Dec 9 2017, 5:31 PM

There's an issue that I've just found out, when saving the fallback profile, named Default, every time I change a setting it creates a new profile 1, profile 2, profile 3. So please hold on the review until I see if I can get this issue fixed.

There's an issue that I've just found out, when saving the fallback profile, named Default, every time I change a setting it creates a new profile 1, profile 2, profile 3. So please hold on the review until I see if I can get this issue fixed.

I tested further, and it appears this ^^^ issue isn't due to this patch. I reverted to konsole5-17.08.1 from the Fedora repos and the same issue happens.

Thanks for working on this.

While testing I notice: 1) rename a profile; 2) hit apply; (appears profile name/file is renamed here) 3) hit OK -> popup saying profile already exists

Thanks for working on this.

While testing I notice: 1) rename a profile; 2) hit apply; (appears profile name/file is renamed here) 3) hit OK -> popup saying profile already exists

I'd hit a similar issue only once; testing again I couldn't reproduce it, nor could I reproduce it in a new user account... :/

Thanks for working on this.

While testing I notice: 1) rename a profile; 2) hit apply; (appears profile name/file is renamed here) 3) hit OK -> popup saying profile already exists

I'd hit a similar issue only once; testing again I couldn't reproduce it, nor could I reproduce it in a new user account... :/

I can reproduce on any profile name change and upon restart of Konsole - so I have the reverse of your testing.. :-)

Current profiles:

/home/kurthindenburg/.local/share/konsole/Dark Solarized.profile
/home/kurthindenburg/.local/share/konsole/Light Solarized.profile
/home/kurthindenburg/.local/share/konsole/New Profile.profile
/home/kurthindenburg/.local/share/konsole/Profile 11.profile
/home/kurthindenburg/.local/share/konsole/Profile 22.profile
/home/kurthindenburg/.local/share/konsole/Profile 3.profile
/home/kurthindenburg/.local/share/konsole/Profile 4b.profile

IIRC, on the account where I had that issue I deleted konsolerc (it's a temp testing account, so I delete config files from there all the time, unfortunately). Could you attach your konsolerc? I am trying to replicate the issue and failing miserably :)

[Desktop Entry]
DefaultProfile=Light Solarized.profile

[Favorite Profiles]
Favorites=Profile 3.profile,Light Solarized.profile,Profile 22.profile

[FileLocation]
scrollbackUseSpecifiedLocationDirectory=file:///

[KFileDialog Settings]
Recent Files[$e]=tabbar2.css,file:$HOME/history.txt,file:$HOME/tabbar2.css

[MainWindow]
Height 1080=632
Height 768=617
State=AAAA/wAAAAD9AAAAAAAAA4YAAAJaAAAABAAAAAQAAAAIAAAACPwAAAAA
ToolBarsMovable=Disabled
Width 1024=905
Width 1920=902

[Notification Messages]
CloseAllEmptyTabs=true

[Profile Shortcuts]
Alt+!=Light Solarized.profile

[TabBar]
TabBarUserStyleSheetFile=file:///home/kurthindenburg/tabbar2.css

I've added a couple of qInfo() calls to the patch I attach here, maybe it'll throw more light on the issue... :/

Sorry for the trouble.

  • Hit Apply after changing profile name

profile->name(): "Profile 11a"
_tempProfile->name(): "Profile 11aa"
otherExistingProfileNames: ("Profile 4b", "Default", "Light Solarized", "", "Profile 22", "New Profile", "Profile 3")

  • Hit OK - popup **

profile->name(): "Profile 11aa"
_tempProfile->name(): ""
otherExistingProfileNames: ("Profile 4b", "Default", "Light Solarized", "", "Profile 22", "New Profile", "Profile 3")

  • Hit Apply after changing profile name profile->name(): "Profile 11a" _tempProfile->name(): "Profile 11aa" otherExistingProfileNames: ("Profile 4b", "Default", "Light Solarized", "", "Profile 22", "New Profile", "Profile 3")

That's it, you have a profile without a name, a file named just ".profile", most likely in ~/.local/share/konsole.

One of the issues this patch is trying to fix is that previously you could remove the name of a profile, you couldn't hit OK, it would display a message saying you can't save a profile with an empty name, but you could still hit Apply, and a ".profile" would still get saved.

I'll update the diff to check if name().isEmpty() before blocking saving and displaying the "already exists" error message. Hopefully with this diff users won't be able to save profiles with empty names.

Thanks for your patience :)

ahmadsamir updated this revision to Diff 23763.EditedDec 11 2017, 4:12 PM

Check if _tempProfile->name().isEmpty before displaying the "already exists" error message and blocking saving the profile after it's been renamed.

So, the issue here is that my Dark Solarized.profile does not have a Name=

I never noticed before - not sure how that happened

When I move that profile out of the way, the current code works - can you still try to handle this issue and perhaps the .profile as well?

[...]

When I move that profile out of the way, the current code works - can you still try to handle this issue and perhaps the .profile as well?

The updated diff should handle both those cases. ".profile" doesn't have a Name=, so basically it's the same as your Dark Solarized.profile, which has a file name but not a Name=.

I've just tested and it seems to work.

I am also thinking that the profile reader shouldn't allow loading a profile without a Name=, sort of a malformed profile, so it should disallow loading it and notify the user to check it manually. (an idea for a future review :)).

Thanks - I notice that renaming a profile loses any shortcuts assigned to it. Do you have time to look at that?

I'll test more in the next few days.

ahmadsamir edited the summary of this revision. (Show Details)

Assign the same shortcut of the old profile to the newly renamed profile.

hindenburg accepted this revision.Dec 12 2017, 2:24 PM

Thanks, looks fine. If I recall you can't commit yourself correct?

This revision is now accepted and ready to land.Dec 12 2017, 2:24 PM

Also noticed this has the same issue as before where a profile is system location (/usr/share/konsole) and the user doesn't have permission to delete it. With this patch, the old profile is left alone w/o any error/message; and the new renamed file is stored in ~/.local/share/konsole

Upon rerunning konsole, both profiles show up.

ahmadsamir updated this revision to Diff 23834.EditedDec 12 2017, 9:25 PM
ahmadsamir edited the summary of this revision. (Show Details)

Add one more check, if the profile config file is not writable (e.g. it's under /usr/share/konsole) don't allow saving, and display a message asking the user to change the profile name if he/she wants to save the settings.

(And yes, I don't have commit access to kde git :)).

ngraham edited the summary of this revision. (Show Details)Dec 12 2017, 11:03 PM
ngraham edited the summary of this revision. (Show Details)

So if I rename a profile under /usr/share/konsole, what do you think should happen? I don't see any changes w/ the latest patch. I can't get the new popup to show up. It still creates the new/renamed profile in .local/share/konsole

We don't need to keep "fixing" issues - the last patch was OK with me; and if you want to work on it further you can do on a clean slate. Let me know either way.

Thanks, looks fine. If I recall you can't commit yourself correct?

Yes :)

Also noticed this has the same issue as before where a profile is system location (/usr/share/konsole) and the user doesn't have permission to delete it. With this patch, the old profile is left alone w/o any error/message; and the new renamed file is stored in ~/.local/share/konsole

Upon rerunning konsole, both profiles show up.

How about changing loadProfile

So if I rename a profile under /usr/share/konsole, what do you think should happen? I don't see any changes w/ the latest patch. I can't get the new popup to show up. It still creates the new/renamed profile in .local/share/konsole

I am going with the logic that users can't save settings to profiles in /usr/share/konsole; so the popup will show if you keep the name unchanged and change any other settings then try to save, it'll block the save and ask the user to change the name. This way there won't be two profiles with the same name in the profile manager dialog.

Does that sound OK to you?

We don't need to keep "fixing" issues - the last patch was OK with me; and if you want to work on it further you can do on a clean slate. Let me know either way.

As you prefer, you're the one who's going to be doing the actual commits to git, so it's your work flow :)

I'm going to commit a version of the previous patch - I changed the method name; added const and changed the connect. If you want to continue, open a new ticket here. I agree with the plan on what to do for profiles in r/o path. Thanks

This revision was automatically updated to reflect the committed changes.

I'm going to commit a version of the previous patch - I changed the method name; added const and changed the connect. If you want to continue, open a new ticket here. I agree with the plan on what to do for profiles in r/o path. Thanks

Excellent, thank you.