ncssar / radiolog

SAR radio log program
Other
14 stars 3 forks source link

Settings #491

Open erinn opened 2 years ago

erinn commented 2 years ago

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 commented 2 years ago

The easier topic first:

Yes, we do use two com ports. Our trailer has two mobile radios at the radio operator station. We go DB25 --> DB9 --> USB-A on each one, then combine those both on a cheap USB hub outside the computer so that we only have to plug in one USB-A connector to the computer, but it's still two COM ports, one per radio. It can certainly operate with zero or one USB connection - but this is basically why we have two green lights at the top left of the radiolog main window. When it comes to the fs send functionality, if no acknowledge is received on the first com port, then it assumes the portable radio in the field is probably not tuned to the same channel as is the mobile radio on that first com port, so, it tries the other com port. If it got an ack on the second com port, then it remembers to try that com port first, next time something is sent to that portable radio. It's a runtime per-device determination. It does save a lot of time to not try again on the radio that didn't work last time. BUT, there's also a lot of the com port listening logic that I had a hard time following, even though I wrote it 7 or so years ago and I know it made good sense at the time. So I'm pretty hesitant to rework any of it until/unless a specific need arises. I did some deep diving into it earlier this year when adding the fs send stuff, and it wasn't that bad, but I'm glad that water is under the bridge.

As for the settings, I'm probably under-educated on the topic of the right place to put settings like this. In general, mention of the registry gives me the heebie geebies. That's probably a culturally ingrained holdover from the earlier days of Windows when you would get a BSOD if someone two rooms down the hall muttered the word 'registry' while sneezing. Even now Microsoft is vocal about saying that one could easily brick one's computer by touching the wrong thing in the registry.

Surely QSettings has addressed most or all of those concerns, so that one wouldn't need to go in to regedit and futz with anything directly. Still, I'm hesitant. Trying to analyze why that is, it's probably just lack of familiarity. What's the big advantage of the registry vs .ini? Seems on the surface that .ini is more portable, more universally editable, more readable, etc. The configparser module works pretty nicely, at least a few other repos under ncssar use it, but radiolog hasn't been changed to use it. Sorry if you mentioned it earlier - I did a pretty thorough brain wipe over the past month - but, have you considered .ini with configparser?

On Mon, Sep 5, 2022 at 4:34 PM Erinn Looney-Triggs @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEPCEZXY2YLYABIGGQI7NV3V4Z7QZANCNFSM6AAAAAAQFKYTAM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

caver456 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Huh - I didn't realize you could tell it to always do .ini - if you mentioned that last month, sorry that must have gone bye-bye in the brain wipe. That fact probably addresses all my hesitations. I guess the subsequent question would be 'what's the benefit of native format vs .ini-always' - they speak to that a bit in the docs, but, nothing seems compelling, so it seems like .ini-by-whatever-means is the winner, and may as well go with QSettings in that case. Lemme read up and ponder a bit more.

As far as a GUI, right now the only settings that get saved are in the optionsDialog, whether modified at startup or at any point during the session. You mentioned the need to have a separate dialog that more directly edits the .ini file, and/or a separate dialog for startup options vs runtime options? I'm not clear on the need for an additional dialog.

(This is separate from the .rc file which keeps track of the state of window locations and sizes and such - maybe rolling those into the .ini file would work too - I guess the original intent was that .rc is the more fleeting transitory 'settings' and the .ini is more of the longer-lasting less-frequently-changed 'configuration' - but that's just semantics.)

Interesting though, with #483 https://github.com/ncssar/radiolog/issues/483 it could actually be useful to have a new prompt that only happens at startup to let the user specify if this session is the next op of an ongoing search. That question doesn't need to show except at startup, and it wouldn't directly affect anything stored in .ini. Also, haven't thought much about the logic of #483 yet so it might not be necessary to prompt the user with that question at all, but rather it could just look for recent radiolog files and prompt the user for confirmation.

On Tue, Sep 6, 2022 at 7:23 AM Erinn Looney-Triggs @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1238224368, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEPCEZVVWKSPA3TDO5S2O43V45HWBANCNFSM6AAAAAAQFKYTAM . You are receiving this because you commented.Message ID: @.***>

caver456 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

The standardized config dir will be great. Good point with the wrinkle, but it's a small enough user base we can probably just hand-edit-and-move the .ini file to do the conversion as needed, and then distribute by hand to all the computers that use radiolog. It would be handy to have an automatic converter function, maybe before the next release or such, but we can cross that bridge later if at all.

I think hand-editing is fine for now, another case of crossing that bridge later if at all. So let's not add a dialog that corresponds to the ini file for now. I think presenting the operator with a GUI method to modify the IP address or the logo icon name or the second working directory or such - even if it's somewhat hidden - would lead to confusion and breakage, specifically because there isn't much tech savvy on our team / on most teams like you say. Those are the things that we want the operator to not see normally if ever. Fewer options is better. I think that was part of the design philosophy behind sartopo, which opens it up to a much larger audience than something like arcgis / mapsar. I think the settings that we want the operator to be able to change - like coord format or datum - are already covered in the optionsDialog. Adding the ability for non-techies to edit the nerdy stuff via the GUI would just be dangerous.

On Tue, Sep 6, 2022 at 11:27 AM Erinn Looney-Triggs < @.***> wrote:

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 on linux 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.

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1238507479, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEPCEZXO2OLODPKZU3IYCJTV46EIJANCNFSM6AAAAAAQFKYTAM . You are receiving this because you commented.Message ID: @.***>

johnpictin commented 2 years ago

Hi Tom and Erinn

If I can weigh in with a techies POV, being in SAR command 90% of the time I see that the KISS principle should be applied. Many of the people assigned to the radio position can type, but they are not as tech savvy as they think they are. And in the downtime between calls they sometimes poke around trying things. It has created a mess.

Any settings accessible in the GUI should be cosmetic or basic preferences and not be anything that could break the system. If you wanted to have a second Python app with a simplistic GUI to edit all of the settings and options or leave it as a documented ini, either one is great. As long as it is not buried in a hidden folder somewhere.

That's my two kb worth.

John

On Tue, Sep 6, 2022, 11:39 AM Tom Grundy @.***> wrote:

The standardized config dir will be great. Good point with the wrinkle, but it's a small enough user base we can probably just hand-edit-and-move the .ini file to do the conversion as needed, and then distribute by hand to all the computers that use radiolog. It would be handy to have an automatic converter function, maybe before the next release or such, but we can cross that bridge later if at all.

I think hand-editing is fine for now, another case of crossing that bridge later if at all. So let's not add a dialog that corresponds to the ini file for now. I think presenting the operator with a GUI method to modify the IP address or the logo icon name or the second working directory or such - even if it's somewhat hidden - would lead to confusion and breakage, specifically because there isn't much tech savvy on our team / on most teams like you say. Those are the things that we want the operator to not see normally if ever. Fewer options is better. I think that was part of the design philosophy behind sartopo, which opens it up to a much larger audience than something like arcgis / mapsar. I think the settings that we want the operator to be able to change - like coord format or datum - are already covered in the optionsDialog. Adding the ability for non-techies to edit the nerdy stuff via the GUI would just be dangerous.

On Tue, Sep 6, 2022 at 11:27 AM Erinn Looney-Triggs < @.***> wrote:

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 on linux 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.

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1238507479, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AEPCEZXO2OLODPKZU3IYCJTV46EIJANCNFSM6AAAAAAQFKYTAM

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1238519815, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM6GMSW5FKBZVDQFHINHYU3V46FX3ANCNFSM6AAAAAAQFKYTAM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

erinn commented 2 years ago

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 commented 2 years ago

Originally (2014 or so), back when our team was on NAD27 CONUS, there were plans to let the user switch between WGS84 and NAD27 CONUS. You saw the various conversion code all commented out. Converting all the table entries for RADIO LOC on the fly / as they are scrolled to would probably be the slowest part, but at least now it uses pyproj4 which would make that more feasible. However, I haven't ever heard anyone ask for it. So, the Datum option is vestigial and grayed out. It could conceivably be revived (or actually just vived), but even if it appears as a hardcode, it's a good visual reminder to the operator that WGS84 is being used.

So, I didn't notice this til now, but: the coordinate format field was actually working a year ago, and now is disabled. The county to our south uses UTM 7x7 and the county north of us uses DMm. So, it's definitely somewhere in the master-cj branch and I'll need to find the code to bring it forward to the master branch. Will make a separate issue.

An audit of all the things in radiolog.cfg (soon to be radiolog.ini) - shows exactly what you said - indeed it really points out that .ini stores the defaults, it does not store the state. There is no code to write the .ini file at all. That's probably the biggest difference between radiolog.cfg in the 'local' dir, vs radiolog.rc in the working dir. Keeping that separation - one file to store defaults, and one file to store the state (usually at shutdown time) - was probably part of the original intent. Not sure whether it makes sense to keep that separation now or not.

Currently editable from optionsDialog:

Not editable from anywhere in the GUI - for techies only, who can edit the .ini file directly:

On Wed, Sep 7, 2022 at 2:41 PM Erinn Looney-Triggs @.***> wrote:

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?

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1239923409, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEPCEZUC53KDEVMNZDRIG3TV5EDZRANCNFSM6AAAAAAQFKYTAM . You are receiving this because you commented.Message ID: @.***>

caver456 commented 2 years ago

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 commented 2 years ago

Not generally a problem at all. No need to wait on me.

From: Tom Grundy @.> Sent: Saturday, September 10, 2022 9:59 PM To: ncssar/radiolog @.> Cc: Erinn Looney-Triggs @.>; Author @.> Subject: Re: [ncssar/radiolog] Settings (Issue #491)

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.

— Reply to this email directly, view it on GitHub https://github.com/ncssar/radiolog/issues/491#issuecomment-1242880473 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AAALGQ6TL377CWSJY4OJO3TV5VKHTANCNFSM6AAAAAAQFKYTAM . You are receiving this because you authored the thread. https://github.com/notifications/beacon/AAALGQ3DGKTTCBEUIBOXNMDV5VKHTA5CNFSM6AAAAAAQFKYTAOWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSKCTM5S.gif Message ID: @. @.> >

caver456 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.