| hindenburg |
| Konsole |
At the moment when a user edits a system-wide color scheme the changes are save in ~/.local/share/konsole; if the user wishes to reset the changes to that scheme and restore the original settings of the scheme in /usr/share/konsole he would have to "remove" the color scheme and restart konsole for the original color scheme to show in the ColorSchemeManager. Now there's a "Defaults" button in the ColorSchemeManager to reset a color scheme to the default values.
This fixes bug 369481 [1]
| Lint Skipped |
| Unit Tests Skipped |
Can you update the patch fixing my comments and updating your master branch - I've made some changes since this patch - although it applies cleanly
| src/ColorSchemeManager.cpp | ||
|---|---|---|
| 226 ↗ | (On Diff #24498) | minor - align indents |
| src/ColorSchemeManager.h | ||
| 95 ↗ | (On Diff #24498) | If possible, use API doxygen labels - here for example use @p name to explain how name is used. |
| src/EditProfileDialog.h | ||
| 135 ↗ | (On Diff #24498) | You're actually deleting the file in .local/share - not overriding it. |
I've addressed all the issues you pointed out (and one more comment issue in ColorSchemeManager.cpp, I used delete instead of overwrite as it indeed explains what's going on in a more accurate way).
GenericDataLocation "~/.local/share", "/usr/local/share", "/usr/share"
For the comments, I think you can use something like "user's home location" and "system locations"
Also, can you "close/mark done" the 3 comments I made earlier that you fixed?
Thanks
| src/ColorSchemeManager.cpp | ||
|---|---|---|
| 240 | I don't think you need to check the isEmpty(), just use the .count | |
Thanks for working on this
| src/ColorSchemeManager.cpp | ||
|---|---|---|
| 224 ↗ | (On Diff #24498) | I'm not sure I understand this - how can the colorscheme file not exist? and why return true when it does not exist? can't you just return dirInfo.isWritable(); ? |
| 240 ↗ | (On Diff #24498) | actually you could use return (paths.count() > 1) |
| src/EditProfileDialog.cpp | ||
| 958 ↗ | (On Diff #24498) | can you name this better than on? isResetable? I'm not sure the "Reset" is the best way to describe what we're doing. Nothing comes to mind though. |
| src/ColorSchemeManager.cpp | ||
|---|---|---|
| 224 ↗ | (On Diff #24498) | Right. (I was over-engineering apparently). |
| 240 ↗ | (On Diff #24498) | OK. Done. |
| src/EditProfileDialog.cpp | ||
| 958 ↗ | (On Diff #24498) | I changed it to use isResettable. About the name, "Reset", it's either that or DiscardColorschemeChanges, which sort of means the same thing... |