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

get font style from main config #523

Merged
merged 3 commits into from
Jun 27, 2023
Merged

get font style from main config #523

merged 3 commits into from
Jun 27, 2023

Conversation

ebisu4
Copy link
Contributor

@ebisu4 ebisu4 commented Jun 18, 2023

While font size changes, there's an issue with setting the font name. Help would be appreciated.

Example:
Append the following to conf.toml located in one of the directories labeled accordingly
Non-Tinas:
$HOME/.config/piker/conf.toml
Tinas:
%AppData%\piker\conf.toml

Append:

[ui]
font_size = 12
name = 'Monospace'

@goodboy
Copy link
Contributor

goodboy commented Jun 19, 2023

@ebisu4 thanks for starting this!

We've been needing someone to help bring up support on the windows front and getting (hidpi) related text and graphics sizing correct would sure be a good start 😂

I'm going to try this on linux and report back; i'm not sure i can comment on why a certain font wouldn't get used on windows.

Maybe there's something we can web search up to do with Qt that's a known hot tip?


@sirdinkus since afaik you would still be on windows, would you mind testing this out?

self._custom_ui = False
# Read preferred font size from main config file if it exists
conf, path = config.load('conf', touch_if_dne=True)
ui_section = conf.get('ui')
Copy link
Contributor

Choose a reason for hiding this comment

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

@ebisu4 only thing i would suggest with this code is add some type annotations:

ui_section: dict = conf.get('ui')
...

name: str = 'Hack'

etc..

@goodboy
Copy link
Contributor

goodboy commented Jun 19, 2023

Ok on linux first try i get this:

(venv)  >>> piker -l info chart --pdb btcusdt.binance
Traceback (most recent call last):
  File "/home/lord_fomo/repos/piker/venv/bin/piker", line 33, in <module>
    sys.exit(load_entry_point('piker', 'console_scripts', 'piker')())
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/lord_fomo/repos/piker/venv/lib/python3.10/site-packages/click/decorators.py", line 38, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/home/lord_fomo/repos/piker/piker/ui/cli.py", line 176, in chart
    _main(
  File "/home/lord_fomo/repos/piker/piker/ui/_app.py", line 184, in _main
    run_qtractor(
  File "/home/lord_fomo/repos/piker/piker/ui/_exec.py", line 167, in run_qtractor
    _style._config_fonts_to_screen()
  File "/home/lord_fomo/repos/piker/piker/ui/_style.py", line 214, in _config_fonts_to_screen
    _font.configure_to_dpi()
  File "/home/lord_fomo/repos/piker/piker/ui/_style.py", line 119, in configure_to_dpi
    self._set_qfont_px_size(self._font_size)
  File "/home/lord_fomo/repos/piker/piker/ui/_style.py", line 81, in _set_qfont_px_size
    self._qfont.setPixelSize(px_size)
TypeError: setPixelSize(self, a0: int): argument 1 has unexpected type 'str'

Was borked on linux if you didn't provide the setting in `conf.toml` due
to some logic errors. Fix that by rejigging `DpiAwareFont` internal
variables:

- add new `._font_size_calc_key: str` which was the old `._font_size`
  and is only used when no explicit font size is set by the user in the
  `conf.toml` config:
  - this is the "key" that is used to lookup a calculation function
    which attempts to compute a best fit font size given the measured
    system displays DPI settings and dimensions.
- make the `._font_size: int` the **actual** font size integer that is
  cached and passed to `Qt` to set the size.
  - this is overridden by user config now if defined.
- change the input kwarg `font_size: str` to the constructor to better
  change the input kwarg `font_size: str` to the constructor to better
  named private `_font_size_key: str` which gets set to the new
  `._font_size_calc_key`.

Also, adjust all client code which instantiates `DpiAwareFont` to use
the new `_font_size_key` kwarg input so nothing breaks XD
@goodboy
Copy link
Contributor

goodboy commented Jun 19, 2023

@ebisu4 made a PR to fix your branch:

ebisu4#1

Copy link
Contributor

@iamzoltan iamzoltan left a comment

Choose a reason for hiding this comment

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

seems like a good idea. havent tried it though. I would add a section in the example config file

@goodboy
Copy link
Contributor

goodboy commented Jun 20, 2023

I would add a section in the example config file

@iamzoltan i did in my sub-PR if you see ebisu4#1

Fix reading font size from user config on linux
@goodboy
Copy link
Contributor

goodboy commented Jun 21, 2023

Not sure why this got closed @ebisu4 but with my merged changed to your fork we should be able to get this tested quick by other devs 😎

@goodboy goodboy marked this pull request as ready for review June 21, 2023 15:43
[ui]
# set custom font + size which will scale entire UI
# font_size = 16
# font_name = 'Monospaced'
Copy link
Contributor

Choose a reason for hiding this comment

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

@ebisu4 tellin me there's issues getting this to work on windows @sirdinkus, so if you could test it a bit it'd be handy 😉

# font_size = 16
# font_name = 'Monospaced'

# colorscheme = 'default' # UNUSED
Copy link
Contributor

Choose a reason for hiding this comment

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

these are just "wishlist" stuff rn since nothing implemented but figured i might as well toss em in for a keen piker to take a swing at 😉

name: str = 'Hack',
font_size: str = 'default',
_font_size_key: str = 'default',
Copy link
Contributor

Choose a reason for hiding this comment

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

main init change was this, font_size is not really what it was 😂 ; more of a name to map to a font size calculator..

anyway, changed all dep code as well so should be fine to land without regression.


) -> None:

self._font_size_calc_key: str = _font_size_key
self._font_size: int | None = None
Copy link
Contributor

Choose a reason for hiding this comment

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

this now gets cached (after first calc) if not defined in the user config conf.toml

TODO: ideally in the future we can actually build an entire "dynamic resizing system" + controls so that a user can do the classic ctl and +/- to change the font sizing (and thus all related graphics) interactively from the GUI. Not sure yet how difficult that's going to be to implement but in theory not that bad since pretty much all widgets have resize handlers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm maybe i'll make a follow up task-issue for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, another thing was checking out that https://github.com/samuelcolvin/watchfiles lib to see if we could also get a "changes-in-conf.toml-triggers-live-update" type thing (like alacritty) workin 😎

Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned in #221 since we're more or less officially exposing a top level config now.

@@ -99,6 +113,14 @@ def configure_to_dpi(self, screen: QtGui.QScreen | None = None):
listed in the script in ``snippets/qt_screen_info.py``.

'''
if self._font_size is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm maybe it's handy to eventually add a .reset() meth as well in case the window gets moved to another display and we want to re-compute the dynamic-by-DPI font size?

(will stick in follow up issue as per above comments)

self._set_qfont_px_size(self._font_size)
return

# NOTE: if no font size set either in the [ui] section of the
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe document this impl detail: the dynamic size calculating by DPI, in the class doc string?

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

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

Other then a follow up issue for the stuff i mentioned in the comments, this looks fine to me.

I'd prefer we have at least one other user try this on windows before landing (and of course on linux) but not critical.

@goodboy goodboy mentioned this pull request Jun 21, 2023
5 tasks
@goodboy goodboy merged commit a65910c into pikers:master Jun 27, 2023
4 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.

3 participants