Move geometry/server version from nextcloud.cfg #5367
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5367 +/- ##
==========================================
- Coverage 58.30% 57.96% -0.34%
==========================================
Files 141 139 -2
Lines 17990 17740 -250
==========================================
- Hits 10489 10283 -206
+ Misses 7501 7457 -44
|
| [[nodiscard]] QString configFile() const; | ||
| [[nodiscard]] QString localConfigFile() const; |
There was a problem hiding this comment.
It is impossible to understand what the difference between configFile and localConfigFile is from their names, and without really digging into the code. localConfigFile should have a name that is more indicative of the data it will hold
There was a problem hiding this comment.
That's true. I hope for suggestions about more adequate name
| if(!QDir(settingspath).exists()) { | ||
| QDir().mkdir(settingspath); | ||
| } | ||
| QString filename = settingspath + QLatin1String("/") + QLatin1String("settings.cfg"); |
There was a problem hiding this comment.
Similarly to the localConfigFile not being expressive of what this file is for, settings.cfg has the same issue
|
I think a good question to ask is do we even need to store the geometry information? This is only used to position the settings dialog and I don't imagine this is really necessary. @mgallien thoughts? |
Signed-off-by: Tomasz Okun <[email protected]>
tokun-mobica
force-pushed
the
feature/Move-geometry-and-serverVersion-from-nextcloud.cfg
branch
from
ab960a9 to
c8681ba
Compare
Signed-off-by: Tomasz Okun <[email protected]>
I would imagine that some might consider this feature as a given especially because I think Qt dialogs and most windows in KDE Community software do store that info between application/dialog runs. |
|
AppImage file: nextcloud-PR-5367-0d2b9d26cb045a3f9b839acee1a6c5ce8a32eca2-x86_64.AppImage |
| @@ -363,6 +363,17 @@ QString ConfigFile::configPath() const | |||
| return dir; | |||
| } | |||
|
|
|||
| QString ConfigFile::localConfigFile() const | |||
| { | |||
| const QString settingspath = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation); | |||
There was a problem hiding this comment.
AppDataLocation is the wrong path here.
Use QStandardPaths::ConfigLocation.
Read the Qt docs for more:
https://doc.qt.io/qt-5/qstandardpaths.html#StandardLocation-enum
|
I think the lingo in this PR is wrong. Geometry and server version are part of the application state and not the application config. The application state should go to |
|
Hello, Thank you for your contribution to the Desktop Client with this pull request. We truly value your support and hope to see more contributions from you in the future! Best regards, |
|
The reference issue isn't fixed (only halfway) so it's still relevant. |
PR related to Remove geometry and serverVersion from nextcloud.cfg