Project Management - Settings Management Clean-Up #3317
Replies: 23 comments 26 replies
-
I'm happy to help here document and slap up a Wiki page (maybe we need a developers wiki area too?) and help trawl through code. |
Beta Was this translation helpful? Give feedback.
-
|
Just to point out, we do have both "Users" and "Developers" section/topics on our wiki front page already. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Completion of first step. From the
I'll spare everyone my immediate observations but there are some serious messiness. Work continues... |
Beta Was this translation helpful? Give feedback.
-
|
Cool, this looks like a very organised approach. Should produce a good result. 😄 And yeah, from that Usage.txt, it does seem like an er... non-trivial amount of work. Hopefully after you get the initial pieces into logical shape, the rest fall into line easily without too much hairiness. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Which has merit. Although INI-format leverages existing API usage from Qt for the desktop application. Key/Value database creeps into NoSQL territory. |
Beta Was this translation helpful? Give feedback.
-
Thats not a Linux friendly format .. wouldn't it be .toml or something? Have no idea what Macs use .. whatever a .plist is? |
Beta Was this translation helpful? Give feedback.
-
|
And this is what I have been mulling over as I dig out all the locations with default values.... When DB4S is first started, only a minimal number of settings are populated. On exit, these minimal settings are stored. It is only after activation of the settings dialog is fulsome, complete, settings populated/saved. I commented as such in the original bug report. The original bug report is derived from the setting for icons not being available or persistent. The code is littered with enormous messiness for settings management. This can be cleaned up. My thoughts currently was to define complete This mechanism would ensure that if app behavior depends on a setting, it is defined in the As for file format, INI format is currently used because it is the default/supported format within Qt. JSON has it's limitations but is most functional. We already make use of the JSON library within DB4S - extending to settings is possible. If we switch to TOML, we would require importing another library into the DB4S code base, sub-optimal situation unless we have other TOML usage. I am incredibly reluctant to use a SQLite DB for settings. It would be a bit awkward to work with, IMHO. |
Beta Was this translation helpful? Give feedback.
-
|
@chrisjlocke We use TOML for the DBHub.io related configuration files as it works pretty well and is fairly popular in Go circles for that purpose. Lots of other projects use JSON files instead, as that's the preference of their authors. Some places do use .ini files, it's not super common, but it does happen. 😉 It doesn't really matter, as they all work. 😄 |
Beta Was this translation helpful? Give feedback.
-
@scottfurry No worries. This does sound like a solid, comprehensive approach. It'll probably take a while, but the end result sounds like it'll be very good. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Throwing this in here more of an observational point... it sort of belongs here but not entirely. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, it's a leftover from when the application used to be hosted on SourceForge, years ago. When we went to update it, it turned out people's preferences were stored under that key. So if we change it, people will lose all their saved preferences. Thus us leaving it alone. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Well, if the whole thing is getting an overhaul, the application can read in from old point A, old point B, old point C, but write to new point D. Mind you, everything else is 'sqlitebrowser' which isn't even our name, so we may as well keep it .. why move to something equally as broken! 🤣🤣 |
Beta Was this translation helpful? Give feedback.
-
Yup, this is a holder-over from archaic times. Various libraries, include macOS, stipulate that a reverse-url should be used to identify storage. The thinking was that since a domain is unique, its reverse should also be unique. With uniqueness, storage of one applications settings should not conflict with other applications. This can be easily fixed. Currently, DB4S has its own domain and we should use that. A wizard-like shim should be developed. User is asked, "Do you want to import old settings?" or "Start fresh from scratch?". Wizard would either move the old settings to a new location or remove the old settings entirely. Hunting down usage of |
Beta Was this translation helpful? Give feedback.
-
Heh Heh Heh. Well, if you're up for it, why not? 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Settings_Usage.ods Second Stage: Steps to regenerate are documented in files Usage_GetValue.txt and Usage_SetValue.txt Some Quick Observations:
|
Beta Was this translation helpful? Give feedback.
-
|
DB4S_Set_Cleanup.diff.zip First swipe at Third Stage - change. DB4S_Set_Cleanup.diff.zip is a zip file of diff from current HEAD. I have replaced all the hard coded strings with constants in a namespace in The original bug of not changing icon style despite user settings is still present. However, a couple of the poorly regulated hard-coded strings (see A difficulty I am finding is a conflation of concerns. User settings are twisted around app presentation/colour settings with SQL window Scintilla syntax highlighting. My personal preferences is to separate these things out. Application Styling in its own file(s) as part of the application install. Syntax Highlighting would be in another file(s) also installed. User could import other styles as desired. This path is a large bit of scope creep, IMO. However, I want to believe things are going in an okay direction. I needed to do some debugging/tracing to get a better appreciation of what is happening under the hood when it comes to user settings. Suggestions welcomed. |
Beta Was this translation helpful? Give feedback.
-
Release v1 of the above fixes to resolve the current setting problems / bugs.
I assume this is easier to spot and rectify once a 'central' settings strategy has been defined? Is it clear /why/ this is still present? |
Beta Was this translation helpful? Give feedback.
-
|
The setting value is used when preferences are updated/changed. My suspicion is that the setting is not pulled in when the MainWindow is established on start-up. Tracing/Debugging should flush out where the change needs to be made. |
Beta Was this translation helpful? Give feedback.
-
|
From #3403 - MacOS does not have a font named :. A default, platform-specific font will need to be defined in settings to avoid. Item added to overall list of TODO items. |
Beta Was this translation helpful? Give feedback.
-
|
And this where we dive into the weeds... At A quick run through in a debugger leads me to believe I can make the shadow problem go away - simple rename. However, looking at the function and calling code gives me pause. There is usage of From context, and I could be wrong here, the intent of the code is checking for whether the variable is populated with a usable value for the path to the settings file. Simply checking for Feel free to chime in here folks. |
Beta Was this translation helpful? Give feedback.
-
|
Upon a fresh installation, I assume userSettingsFile is null as its not set .. not until a file has been created or the user specifies one at startup? Do we therefore expect the 'is this a valid name to a file' to legitimately fail? |
Beta Was this translation helpful? Give feedback.
-
|
assume - such a dangerous word in this day and age... However, if we test for |
Beta Was this translation helpful? Give feedback.
-
|
Taking another swipe at changes. Code "looks" better. I can rationalize some of what is being done here. A key issue was the shadowing I mentioned earlier - fixed. If executed from the console, DB4S w/ diff applied spits out when a default value is not found. There are a few The problem with a lack of persistence of some settings keys is still present. Resolving will be another debug session. Feel free to point out issues/problems. |
Beta Was this translation helpful? Give feedback.
-
That's a really smart idea. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
One small step for man...One giant leap forward for me... \o/ There's a bunch of debug statements being thrown to the console during execution. That can be trimmed down. However, I think I have a handle on the default fixed font setting. This should silence the noise over usage of "Monospace" font on MacOS. Feel free to take the patch for a test drive.
|
Beta Was this translation helpful? Give feedback.
-
|
At some point, we're going to have to have a discussion about font usage. I was able to clean-up the warning message in MacOS about missing The application font is determined from the OS settings for a serif font. Then there is the font for the Databrowser, the SQL Editor, the Log, and <others, I'm sure>. Databrowser font is selectable in the preferences dialog, Same also for the Editor. Log, Databrowser, and SQL Editor are set to fallback to the OS defined fixed width font. Future consideration should be given to simplifying font usage/selection. The general tab could be adjusted to have selection for a serif font and fixed width font. Other tabs with font selection would either use the application defined fixed or serif font. At least, that's my thinking. Everyone is welcomed to chime in here... |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, incremental progress... The original bug, #3312, that started me down this path pointed out an issue with toolbar preferences. And speaking of can of code worms (see my previous post about fonts)... Looks like I'm going to have to enforce some kind of order in UI elements. I was so hoping I wouldn't have to touch these... // MainWindow.cpp:2340
}
- setToolButtonStyle(static_cast<Qt::ToolButtonStyle>(Settings::getValue("General", "toolbarStyle").toInt()));
- ui->dbToolbar->setToolButtonStyle(static_cast<Qt::ToolButtonStyle>(Settings::getValue("General", "toolbarStyleStructure").toInt()));
- ui->toolbarSql->setToolButtonStyle(static_cast<Qt::ToolButtonStyle>(Settings::getValue("General", "toolbarStyleSql").toInt()));
+ ui->toolbarDB->setToolButtonStyle(
+ static_cast<Qt::ToolButtonStyle>(Settings::getValue(szINI::SEC_GENERAL, szINI::KEY_TB_STYLE_APP).toInt()));
+ ui->dbToolbar->setToolButtonStyle(
+ static_cast<Qt::ToolButtonStyle>(Settings::getValue(szINI::SEC_GENERAL, szINI::KEY_TB_STYLE_STRUCTURE).toInt()));
+ ui->toolbarSql->setToolButtonStyle(
+ static_cast<Qt::ToolButtonStyle>(Settings::getValue(szINI::SEC_GENERAL, szINI::KEY_TB_STYLE_SQL).toInt()));
// Set prefetch sizes for lazy population of table models
for(int i=0;i<ui->tabSqlAreas->count();++i)There's an ASSUMPTION being made in the original code that calling However, if I remove the assumption and explicitly reference the ui element the preference is persistent on restart. Again, incremental progress... Unfortunately, the ExtraDB and Project toolbar button styles do not appear to be following what is in the preferences. Like I said earlier, can of code worms. |
Beta Was this translation helpful? Give feedback.
-
|
Heh Heh Heh. It's probably a really, really good thing that we have someone who knows the value of persistence working on this. 😁 |
Beta Was this translation helpful? Give feedback.
-
|
Btw, not really commenting technically here because everything you're saying is making sense to me. So, no objections or insights on my part. |
Beta Was this translation helpful? Give feedback.
-
|
Something to be said for having a modicum of obstinance. It's a coin toss between grey-hair induced, pragmatic wisdom and a healthy dose of fear and common sense. Jury is still out on which. |
Beta Was this translation helpful? Give feedback.
-
|
Pull request, #3424 , created to directly address problem with toolbar. Settings affecting toolbar will be cleaned up with this effort. #3312 can be closed. |
Beta Was this translation helpful? Give feedback.
-
|
Appending snapshot of work-in-progress. Diff above includes the QToolBar fix from above, as well as, rework of code as a result of other recent commits. The adventure continues... |
Beta Was this translation helpful? Give feedback.
-
|
Moving #3341, referencing |
Beta Was this translation helpful? Give feedback.
-
|
Just to clarify things...The last few posts in this item are intended to address some Scope Creep Management. Re-assessing what work is involved to correct issues added appears to be more involved than just settings. Dealing with these items immediately, or deferring to the Code TODO List, #3419, as separate matters seems logical and prudent. |
Beta Was this translation helpful? Give feedback.
-
|
The zip above contains TWO diffs. Both focus on implementing a single effort. It helps me wrap my head around what I'm doing, as well as preventing straying into extra editing. First diff is just a straight-up renaming of all the string literals associated with settings storage. The second diff is where some manipulation of the Settings object occurs. Everything still builds somewhat cleanly and no bugs appear to have been introduced. I recently tweaked something in my OS that affects Qt presentation. This has created a glitch with how Qt reports font settings. I have to re-examine font default settings. Another concern is the syntax highlighting. How this is implemented works but seems a touch ugly to maintain. I'm considering if this functionality can be somehow peeled out and detached from settings. Again, not introducing bugs is key here. |
Beta Was this translation helpful? Give feedback.
-
|
Excellent. This is looking like really solid progress @scottfurry. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
...progress... A few points... The function b) The warning... still happens when executing on MacOS. I believe this stems from c) d) I'm still not happy about the code for There is still a bit of work needed but things seem in a much better place. ... and, of course, I'm finding more Code-ToDo's. Shocking, I know (insert heavy sarcasm). |
Beta Was this translation helpful? Give feedback.
-
|
Mechanism to correct for deprecated/bad settings values implemented. On initialization of Settings Object, settings file storage is scanned for section/key pairs that are deprecated or invalid. If item is found the replacement section/key pair is established with found value and old section/key pair is removed. (see For now, only one item is being searched. The key Also, mechanism exists to remove section/keys no longer in use, i.e. ensure unused settings keys do not remain resident, (see Both mechanisms are flexible enough for future changes. The old section/key pair, and replacement for deprecated only, just need to be appended to the appropriate container. Code takes care of the reset. So what's left?
|
Beta Was this translation helpful? Give feedback.
-
|
Cool, solid progress happening. 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Settings_Clean-Up_diffs_2023Sep18.zip And movement. I was able to crawl through syntax highlighting. There was clean-up. Stuff is better. Code feels more solid and, as a bonus, I'm not introducing stuff that shouldn't be there. Last item on my TODO list is taking another swipe at Stand By to Stand By. Edit: Forgot to mention... Had to rebase everything to latest commits. There was a change someone made to set max row count. I missed that commit but was able to incorporate that change into this effort. |
Beta Was this translation helpful? Give feedback.
-
|
The weeds... A machete was involved...I just barely made it out... Having reworked a few things, I was enthusiastically doing my due diligence ensuring everything was working just fine. Then I got sucked into the vortex that is Qt's QSettings API. It wasn't pretty. So, what I discovered is that Qt considers the section name "General" to be a reserved name. Any attempt to change will not work. QSettings will automatically rename the section as "%General" and save the result. Of course, the whole matter is undocumented. Shocking, I know <insert heavy sarcasm here>. I can sort of see why from Qt's perspective. At the same time, I can see how DB4S code has been configured to work with Qt's imposition. The overwhelming urge to re-write things was enormous. I was seriously questioning the whole validity of the exercise thinking I was misunderstanding the read/write of settings. Again, ugly. I have some changes to undo. And a bug report to file (Damn it. Document this quirk Qt!). Edit: FTR, QT Bug 117427 |
Beta Was this translation helpful? Give feedback.
-
|
Pull Request #3481 - It is done... |
Beta Was this translation helpful? Give feedback.
-
OverviewLet's wrap this up with a quick discussion about code changes and impact. Start Up and Settings InitializationOn App start the environment variable The settings object is started. The settings are then scanned to remove deleted settings values ( Get ValueThis mechanism is reworked a bit. The order of preference is still to retrieve the cache value before accessing the settings file. Added is the mechanism that if neither contains the desired section/key value, the application falls back to a default value ( Going ForwardSettings values will need to be managed in future. INI file section/keys are explicitly defined as New Section/Keys will need to be added to the If the new key involves fonts or colours, entry default should be defined as an empty QVariant and then populated in the Finala) QHexEdit is causing noise about the "Monospace" font. This stems from the current version of code having an if statement that explicitly sets the control font to "Monospace" for non-Windows platforms. I believe this issue is corrected upstream in later versions. For MacOS platforms, this warning can be ignored as the font for the control is set via fixed font default functionality on application start. b) In setting the fixed font, functions from the QFontCombo control were pulled into DB4S source. I find it incredible that useful methods to programmatically determine system fixed font is buried in a GUI control. QTBUG 116223 is a feature request to QT to move the functions |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Topic Rationale
My world currently is a bit messy at the moment. I need to peel off circumstances discovered during investigations associated with an issue. Discussion topic is being used both to ensure I do not lose visibility of what was found triaging identified issue, as well as address other items that exceed scope of issue. I am initially including the issue reporter, @bartek990, in this conversation.
Topic Background
Issue #3312 was created for awareness about problems with DB4S's response to user selected toolbar icon settings. My review of the matter determined that the setting was being established in storage. However, DB4S code may not be making use of this setting value on application start-up. There is some reporting this behavior is intermittent.
However, review of code for DB4S settings and main window appear to show larger problems with how settings are being stored, retrieved, and utilized in DB4S. Specifically, only a bare settings file is established when exiting initial usage of DB4S. Also, some settings values appear to be employed only on the
saveaction of the settings dialog. Finally, I have concerns about setting key/value pairs and how the default values are being documented and/or are scattered across the DB4S code.A comment to #3341 identified usage ofsourceforgedomain for storage of settings. Develop code to detect this circumstance. Effort should be made to removesourceforgereference and utilize current DB4S domain.see commentary below
Pull request #3348 identified issue where a default font is not established for databrowser control on application start-up.
Topic Goal
Ultimately, a clear and consistent settings practice and handling are ideal. Efforts under this topic will work towards incorporating these desired changes.
Steps
Settingsnamespace)This includes platform specific defaults (Wrong display in cells DB Browser for SQLite #3403 identified MacOS does not have a font named
Monospace)- develop a wizard to identify storage withsee commentary belowsourceforgedomain. User to be given a choice: a) leave existing settings and use; b) leave existing settings and start fresh; c) move existing settings to new location; or d) remove existing settings and start fresh.I will keep others apprised of efforts on this front as my time permits.
Beta Was this translation helpful? Give feedback.
All reactions