Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settings #491

Open
erinn opened this issue Sep 5, 2022 · 16 comments
Open

Settings #491

erinn opened this issue Sep 5, 2022 · 16 comments

Comments

@erinn
Copy link
Contributor

erinn commented Sep 5, 2022

Welcome back and thanks for merging that. So one of the things we have sort of discussed on and off is the settings and where they are stored. I think you and I are on the same page that the settings should be moved on out of the installation directory and I've been thinking on how to do that. However, I want to make very sure that we are on the same page about this next idea because it will require a fairly significant amount of work I imagine.

My thought here is to move the settings into QSettings (as I've mentioned). This means on windows at least they will be stored in the registry. This is probably more 'proper' but may not be 'right' for your overall design, I don't know. If settings were to be moved from the INI files into the registry this would also imply/require a settings dialog so the user can interface with and change the settings. You currently have your 'options.ui' dialog that is accessed via the sprocket or on first start up. This is sort of close to a settings dialog, but my feeling is you need a 'New Incident' dialog and then a settings dialog separately, not rolled into one.

Along these lines one of the things that has puzzled me in the code base is the, 'fsFirstPortToTry' and 'fsSecondPortToTry' dance that is done. I can't really tease out from the code why you are doing that exactly. You are looking for the most recent com port that was used, but why? Do you run with two com ports regularly? Would it make more sense to have the settings dialog allow for the two com ports and be able to check one as the 'send port'?

Anyway, let me know your thoughts before I dive in, because this is more invasive than anything else I've done.

@caver456
Copy link
Collaborator

caver456 commented Sep 6, 2022 via email

@caver456
Copy link
Collaborator

caver456 commented Sep 6, 2022

This came up when googling 'qsettings vs ini':

https://stackoverflow.com/questions/57359518

It's a good discussion but I'm still leaning towards .ini and configparser. They don't really discuss the portability advantages of .ini. One I hadn't thought about is that .ini is consistency across platforms. You could copy an .ini from Windows to Mac and it would just work.

The ability for QSettings to encode and save QVariant objects is interesting, but I don't think we'd need anything like that now...? I think doing so would go beyond the scope of what an .ini is generally considered to be meant to do anyway, and more into the realm of saving the state of things.

But, if I'm missing something in those considerations, please do let me know. Wouldn't be the first time!

@erinn
Copy link
Contributor Author

erinn commented Sep 6, 2022

So like a lot of things these are just matters of opinion mostly. QSettings is not some amazing silver bullet as far as I can tell, but it does leverage the QT framework that you have already built so much on top of. I should also mention that there is nothing wrong with INI files, in fact you can use qsettings to store all settings on all platforms in INI files: https://doc.qt.io/qt-6/qsettings.html#Format-enum

To me, using QSettings feels, as I said, more proper, but again that doesn't make it right. Even using QSettings and just having it backend into INI files feels more proper because you get the 'proper' locations to save the INI file built in: https://doc.qt.io/qt-6/qsettings.html#platform-specific-notes

All that being said, nothing wrong with just plain INI files and parsing them. You just need to decide where to put them in a way that fits in with multiple platforms (potentially, I'd love to see this working on Mac and Linux). Which QT already does for you, which in turn can make life easier :).

So all in all, do you want a settings dialog to be able to modify these settings? Or would you rather folks dive into editing an INI file to make changes? Your call.

As for the com ports, that makes sense. For our use case we'll only be using one port, but that code just looked complicated and fragile so I thought I would ask.

@caver456
Copy link
Collaborator

caver456 commented Sep 6, 2022 via email

@caver456
Copy link
Collaborator

caver456 commented Sep 6, 2022

After some more googling and RTFM, thumbs up on QSettings with IniFormat.

Thumbs up also to rolling the current .rc file data into the same .ini file, but would prefer that to be a separate PR i.e. one thing at a time.

Thanks for bringing this up / forward.

@erinn
Copy link
Contributor Author

erinn commented Sep 6, 2022

Ok. So I can work to move things to qsettings, that is fairly simple with one wrinkle, your settings file location is now changing, which in turn means you'll lose your settings. This may not be a big deal with a small user base or a transition needs to be worked out.

As for the settings dialog. Again everything is opinion. To me this is a GUI application and a user who starts it probably has a reasonable expectation that they should be able to modify settings via the GUI not via editing a file directly. There is nothing wrong per se with the latter it just tends to limit your user base, and SAR being SAR, they need all the help they can get with tech in my experience. So it's my opinion that exposing the settings via a dialog makes this more approachable etc. But again, your project, so your call.

@erinn
Copy link
Contributor Author

erinn commented Sep 6, 2022

And one final word here for the moment. INI versus native, I don't have a good clean answer for you here just a suspicion. I suspect that native format works better when you are 'closer' to the OS than you are when using QT. I suspect when put through QTs abstraction, it doesn't buy you much to use native format. However, a large number of applications are using the registry/plists/ini as QT is aiming for, which may be more 'proper'. But my main goal here, shorter term is to move all dynamic files out of the install directory so the installer becomes more useful.

@caver456
Copy link
Collaborator

caver456 commented Sep 6, 2022 via email

@johnpictin
Copy link

johnpictin commented Sep 7, 2022 via email

@erinn
Copy link
Contributor Author

erinn commented Sep 7, 2022

I don't personally agree. However, I'm old enough at this point to know that just cause I don't agree doesn't make me correct :). So no worries, no plans to make a settings dialog.

That being said, the settings dialog that currently exists, has never allowed me to adjust the datum or much of anything else for that matter. This seems to be strange to me, but perhaps I am missing something?

@caver456
Copy link
Collaborator

caver456 commented Sep 7, 2022 via email

@caver456
Copy link
Collaborator

I probably asked before, but, how painful to you is it if I tackle some of the other bug fixes and enhancements while you're still working on the settings stuff? I've only ever dealt with a merge conflict once or twice and it was not all that pleasant.

@erinn
Copy link
Contributor Author

erinn commented Sep 11, 2022 via email

@caver456
Copy link
Collaborator

caver456 commented Oct 1, 2022

Hi @erinn I'd like to do a bumpver to make a new release (probably 3.1.0) sometime in the coming week. 31 issues have been closed since July 1, and all of the recent bugs are closed except #482 which is very-very-low-priority. I'd like to tackle a few of the medium-or-high-priority recent enhancement issues before doing the bumpver, but, soon. Do you plan to do a PR for this issue in the next week or so? 1) no rush, 2) we're both volunteers, so level of interest and availability ebbs and flows for sure, 3) I certainly haven't made the workflow terribly easy for other contributors.

@erinn
Copy link
Contributor Author

erinn commented Oct 1, 2022

Funny, was literally just working on this today and thinking two things. One, my it's hard to integrate changes into this moving target. Two, there probably should be a release.

I do plan to do a PR here soon, probably today or tomorrow.

So one suggestion here in terms of branches, good job on using them, I'd recommend deleting the branches in GitHub after you merge them.

Another longer term suggestion would be to break this 6k line file into multiple parts to make it easier to approach and can allow concurrent work in a simpler fashion. I don't claim to be any authority here, but here is an example layout from one of my projects: https://github.com/erinn/kconsole following more along the lines of the model view design.

Anyway, soon here for the settings, I'm actually mainly stuck on readme files and updating those to more closely reflect reality.

@caver456
Copy link
Collaborator

caver456 commented Oct 1, 2022

Will delete my old branches now. I just did some reading on pro's and con's of deleting branches, seems there aren't really any cons except the loss of historical context of the branch, and I guess that's why I was hesitating to delete them so far. But, seems like that historical context isn't really important, since all the rest of the commit history is still there. One post also pointed out that a branch is nothing more than a link to commits, so, there's really no unique value added after the branch is closed.

Breaking up radiolog.py would be great, I definitely get that there are pitfalls to having a single huge file. Someday. The big changes / refactors are still pretty spooky without robust testing. Was thinking about testing the other day: with something that's generally speaking a pretty complex state machine like this, is it actually safer and more thorough and more robust to do a) roll-your-own 'bespoke' testing, with a checklist of scenarios like this issue, or, b) writing the automated testing? Seems like there are pro's and con's both ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants