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

Allow adjusting the fontsize of the pulsectl default device popup #986

Merged

Conversation

TheEdgeOfRage
Copy link
Contributor

@TheEdgeOfRage TheEdgeOfRage commented Aug 30, 2023

I've recently started using the pulsectl tk popup to pick the default device and one annoying thing is that the default font size is way too small for me, so I added basic support for changing it. I'm not sure if this is a good way to implement it, let me know if not :)

Edit: I just remembered that I should probably also update the docs 😅 I'll try to get around to that later today after work

@tobi-wan-kenobi
Copy link
Owner

Nice addition, thank you very much! Great to read from you again!

I think eventually, the best option probably would be having this as global (module independent) parameter. What do you hink?

@TheEdgeOfRage
Copy link
Contributor Author

Oh, I didn't know you can have global ones. That would definitely be preferred then :D

How do I define it globally?

@tobi-wan-kenobi
Copy link
Owner

Those are the parameters defined outside the modules (in config.py, i think, can't remember for sure and don't have a pc handy)

I think popup should then have access to that via a config object.

@tobi-wan-kenobi
Copy link
Owner

Looking at it in a bit more detail:

  • If you look at bumblebee_status/core/config.py, you can see how "interval" is set on a global level, and use it via config.<accessor>(). I think that maybe this would be the best option here.
  • More generally, it might make sense to always recurse to a global parameter, if no more specific one is present (i.e. use pulsein.fontsize, but if that is not found, use fontsize - what do you think?
  • Generally, I think the fontsize logic should be in the popup module, not in the individual widget modules, what do you think?

If you prefer, I can put together a draft PR for those changes, as they are targeting the core quite a bit.

@TheEdgeOfRage
Copy link
Contributor Author

Sorry, I haven't had much time these days to finish this. I've removed the code from the pulsectl module and I agree with your third point that it should ideally all be in config and the popup module. The issue I'm having is that config is a Class definition, but the instance is only available in the main file. Would it make sense to have config be a singleton that can be imported from other modules?

@tobi-wan-kenobi
Copy link
Owner

An instance (the instance, actually) of Config is passed as the first parameter to init() of all modules, iirc. Does that help?

@@ -75,19 +75,15 @@ def update(self):

def popup(self, widget):
"""Show a popup menu."""
menu = util.popup.PopupMenu()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to have been using an old implementation, so I've updated it

@@ -77,7 +77,7 @@ def popup(self, widget):
util.cli.execute(popupcmd)
return

menu = util.popup.menu()
menu = util.popup.menu(self.__config)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the best I came up with for passing on the config object to the popup menu. WDYT?

@tobi-wan-kenobi
Copy link
Owner

Looks good, thank you very much!

@tobi-wan-kenobi tobi-wan-kenobi merged commit b9e45ca into tobi-wan-kenobi:main Sep 6, 2023
3 checks passed
@TheEdgeOfRage TheEdgeOfRage deleted the pulsectl-font-size branch September 6, 2023 15:24
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