aussig / BGS-Tally

A tool to track and report your Background Simulation (BGS) and Thargoid War (TW) activity in Elite Dangerous, implemented as an EDMC plugin. BGS Tally counts all the work you do for any faction, in any system.
https://discord.gg/YDNVtjPnnm
MIT License
31 stars 9 forks source link

make the placement of the tick overlay message configurable #138

Closed demon5760 closed 1 year ago

demon5760 commented 1 year ago

Disclaimer First time I have worked with EDMC Plugins as well as this code base, so I'm open to make any changes you think make sense.

I don't personally like having the Curr Tick message on my screen the entire time, so I set out to make it so I could toggle it on/off via configuration. As I got into it I opted to allow it to be configured for any corner of the screen (which maybe overkill).

image

Example showing it in somewhere other than the current location image

aussig commented 1 year ago

Hi CMDR, thank you very much for the contribution. I like the way you are thinking, and with more and more overlay features being introduced in 3.2.0 and more to come in future versions, we need a way to allow the user to control things on screen.

However, thinking forward I believe an on-screen UI with predefined positions (top-left, top-right etc.) could get cluttered very quickly, considering it would need to cater for all the additional panels, both current and future. Also, I think the 'Tick Warning' box should always be positioned directly below the tick indicator.

I agree it would be good to have on-screen checkboxes to enable / disable each panel in the EDMC settings, but for positioning I was thinking of introducing a configuration to allow the user to define coordinates for each panel in a .ini file, then the plugin would read that on start-up. This is also more flexible as the user has full choice of positioning - this is how other plugins like EDR manage it.

To support this, I will need to extend the .ini mechanism (see #132) - it already reads from the config.ini file, but that needs to be extended to read from a second, user editable one (user_config.ini?) that doesn't get reverted with each new release of the plugin, and is git ignored.

So would you be open to the idea of removing your fixed corner positioning code, reimplementing the 'Disabled' option as a checkbox, and introducing similar checkboxes for the 'Info', 'Activity Indicator', 'Tick Warning', 'Thargoid War Progress' and 'System Information' panels to enable them to be switched on and off?

I'll try to prioritise the .ini file work.

demon5760 commented 1 year ago

Yeap, I would be totally opened to going back to the simpler enabled/disabled checkbox. I will start with the Current Tick only as I am not yet familiar with the other items just yet. And just to make sure we are on the same page Info is something different the Current Tick message? Sorry if its a silly question, I just have not dug into all the "other" messages yet as I was pretty focused on just the one and want to make sure I use the proper terminology.

demon5760 commented 1 year ago

Updated based on current assumptions. It looks like this now: image

Question on verbiage: Do you prefer Show or Enable?

aussig commented 1 year ago

Just realised that because I clicked through straight from the notification email, I missed your two questions in this PR thread:

And just to make sure we are on the same page Info is something different the Current Tick message? Sorry if its a silly question, I just have not dug into all the "other" messages yet as I was pretty focused on just the one and want to make sure I use the proper terminology.

Question on verbiage: Do you prefer Show or Enable?

I prefer 'Show'.

demon5760 commented 1 year ago

All good. I'll incorporate the changes tonight. I appreciate you taking the time to explain what each of those is.

demon5760 commented 1 year ago

OK @aussig - I think this one is ready to go.

aussig commented 1 year ago

OK @aussig - I think this one is ready to go.

Did you push your changes, I don't see a new commit...

demon5760 commented 1 year ago

🤣 🤦 - More coffee next time. actually pushed it this time.

aussig commented 1 year ago

A note for future - your code crashes occasionally because in the ui._worker() method, you are accessing EnableOverlayCurrentTick.get() which is getting a value from a tk variable, which isn't a thread-safe operation. To get around this, note the state.refresh() function which mirrors the values of the tk variables into normal variables (just the one at the moment - self.enable_overlay). Your new tk checkbox with its variable needs a similar mirrored variable for thread-safe access.

I'll fix this one, but thought you might want to be aware for the future :)

demon5760 commented 1 year ago

Oh that's good info. I had looked at that function, but I wasn't entirely sure what is was for and in my local testing it appeared to work the same with and without. Makes sense if its not threadsafe how it would not be obvious. How were you able to get it to crash? I'm curious where I fell short in my own testing to not have seen that.

aussig commented 1 year ago

I ran it for a bit then saw the overlay disappear, think I was looking at the settings too. That was the thread crashing but note the plugin will often survive worker thread crashes unless tk kills the main thread too.

I’ve added show/hide settings for the other overlay panels too now, and threadsafe mirror variables for each - take a look at latest develop, it should be clear how it works.