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

TK-690: Handle memory bank renaming and changing head types. #1205

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

Patronics
Copy link
Contributor

@Patronics Patronics commented Dec 17, 2024

This one isn't ready to merge yet, but I was wondering if I could get some tips on how to handle two inter-dependent settings, which I struggled with in commit 2924127?

Specifically, the channel name length and group name length must add up to the total display length, which is either 8 or 14 characters, depending on which head unit is equipped. I attempted to handle that logic, but haven't found a way to successfully update one setting in the UI when the other one's value changes.

Similarly, even though I set the head unit type to be a volatile setting (as some buttons are only available on the full head unit), there's no change in the list of settings on the button_assignments settings tab after changing the head unit type.

@kk7ds
Copy link
Owner

kk7ds commented Dec 17, 2024

This one isn't ready to merge yet, but I was wondering if I could get some tips on how to handle two inter-dependent settings, which I struggled with in commit 2924127?

Specifically, the channel name length and group name length must add up to the total display length, which is either 8 or 14 characters, depending on which head unit is equipped. I attempted to handle that logic, but haven't found a way to successfully update one setting in the UI when the other one's value changes.

Yeah, volatile is the best we have here, so if you mark them both as such then you can make changes to the other and have the UI refreshed. Might be best to mark one as immutable (set_mutable(False)) and always make it calculated. So the user controls both by just setting one.

Similarly, even though I set the head unit type to be a volatile setting (as some buttons are only available on the full head unit), there's no change in the list of settings on the button_assignments settings tab after changing the head unit type.

Yeah, you can't change the structure of the settings after we've built the UI. TBH, I'd just expose all of them with doc tips that the advanced ones (obviously) don't do anything if you don't have that head. However, I think you might also be able to mark them immutable/mutable based on the setting of the (volatile) head type. If the UI doesn't refresh the mutable-ness when it reloads after a volatile setting change we could probably fix that fairly easily.

The settings stuff is not what I would choose to design today, but some of the complexity comes from the fact that we support "live radios" as well, where settings (and all other) changes get flushed to the radio in real time.

@Patronics Patronics marked this pull request as draft December 18, 2024 08:28
@Patronics
Copy link
Contributor Author

Patronics commented Dec 18, 2024

Thanks for the tips. Setting mutable to false for one of the settings makes the logic a bit simpler. Nothing seems to be reloading with volatile enabled, however. Pressing Command-R to reload the driver does update values accordingly. I'm going to see if I can spot any reasons the UI isn't refreshing as intended when volatile is active.

@Patronics
Copy link
Contributor Author

Okay, did some digging into volatile's implementation, from what I can find, in common.py

chirp/chirp/wxui/common.py

Lines 474 to 478 in 605037d

LOG.info('User made change to %s=%s despite warning',
event.GetPropertyName(), event.GetValue())
self._needs_reload = setting.volatile
if self.needs_reload:
wx.MessageBox(_(
is successfully setting the needs_reload value, but in settings_edit.py
self.do_radio(self._set_settings_cb, 'set_settings', settings)
wx.PostEvent(self, common.EditorChanged(self.GetId()))
if self._propgrid.needs_reload:
LOG.warning('Settings grid needs a reload')
wx.CallAfter(self._reload)
when it tries to check the value from the propgrid, it gets the value as false, so the reload never happens. I'm not familiar with the details of the propertygrid implementation, but hopefully that gets you on the right track to solve it.

@kk7ds
Copy link
Owner

kk7ds commented Dec 18, 2024

The reload handling works differently in the new UI than I remember, I guess I'm thinking about the old UI. So I guess that means you can change the structure, although I kinda feel like trying to keep the structure stable would be better.

I'll have a look at why it's not reloading for you later (no time today) but it's working for me for another radio that uses it.

@kk7ds
Copy link
Owner

kk7ds commented Dec 20, 2024

Okay yep, it was totally broken. Just happened to work for the driver I implemented it for because it was on the last page, which is silly of course. I imagine it was one of those "I'll fix this after I make sure it works" sort of things. So, new change at the front of your stack here. Please confirm it works for you and then I'll push it into the tree separately so you don't have to carry it.

@Patronics
Copy link
Contributor Author

Okay, I just tested your fixes, it works great now, thanks for spending the time to figure it out!

@Patronics Patronics force-pushed the memRename branch 3 times, most recently from c480b39 to 736c45c Compare December 21, 2024 10:24
@Patronics Patronics marked this pull request as ready for review December 21, 2024 10:24
@Patronics
Copy link
Contributor Author

Patronics commented Dec 21, 2024

Okay, I've pushed some changes, adding a few more features (the power-on message and compander setting for each memory). Also fixed an issue I found with the default mode for new memories on the TK-690 model in particular. I think these changes should be ready to merge now :)

@Patronics Patronics changed the title WIP: TK-690: Handle memory bank renaming and changing head types. TK-690: Handle memory bank renaming and changing head types. Dec 22, 2024
Copy link
Owner

@kk7ds kk7ds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Mostly looks good, yep. The nit is not worth holding up over, but the dunder method thing is weird, so I'll hold for that. If you're busy and can just confirm that int() is right, I can fix and merge for you.

Thanks!

chirp/drivers/tk690.py Outdated Show resolved Hide resolved
chirp/drivers/tk690.py Outdated Show resolved Hide resolved
@Patronics Patronics force-pushed the memRename branch 3 times, most recently from f6d8c78 to f521952 Compare December 23, 2024 18:48
kk7ds and others added 2 commits December 23, 2024 10:49
The naive implementation of volatile settings reloading the whole
panel only worked for the last panel in the stack. That happened to
work for the drivers that used these for advanced/weird settings at
the end, but obviously is not correct overall.

This makes us pass the reload flag in the wx event from the setting
grid actually changing so we see it in the editor when we are
processing the change so we can reload when we are done.
added group names,
setting basic/full control head,
setting welcome message,
setting channel/group name lengths,
and compander enable/disable per channel
@kk7ds kk7ds merged commit 855276b into kk7ds:master Dec 23, 2024
6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants