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

feat: add toolbar widget #597

Merged
merged 18 commits into from
Oct 22, 2023
Merged

feat: add toolbar widget #597

merged 18 commits into from
Oct 22, 2023

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Oct 9, 2023

closes #593

this PR includes #598, and should wait for it to merge

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (2fa477d) 87.98% compared to head (7f39313) 87.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #597      +/-   ##
==========================================
- Coverage   87.98%   87.56%   -0.42%     
==========================================
  Files          39       40       +1     
  Lines        4609     4727     +118     
==========================================
+ Hits         4055     4139      +84     
- Misses        554      588      +34     
Files Coverage Δ
src/magicgui/backends/_ipynb/__init__.py 100.00% <ø> (ø)
src/magicgui/backends/_qtpy/__init__.py 100.00% <ø> (ø)
src/magicgui/widgets/__init__.py 100.00% <ø> (ø)
src/magicgui/widgets/_concrete.py 89.45% <100.00%> (+0.04%) ⬆️
src/magicgui/widgets/bases/__init__.py 100.00% <100.00%> (ø)
src/magicgui/widgets/protocols.py 100.00% <100.00%> (ø)
src/magicgui/widgets/bases/_toolbar.py 96.00% <96.00%> (ø)
src/magicgui/backends/_qtpy/widgets.py 87.43% <78.37%> (-0.38%) ⬇️
src/magicgui/backends/_ipynb/widgets.py 61.34% <34.21%> (-4.23%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlambert03
Copy link
Member Author

this is mostly working now for both backends... but doesn't yet have icon support. I might want to add icons in a new PR. superqt recently got support for iconify ... and it would nice to be able to provide the same flexibility for ipywidgets.

@tlambert03
Copy link
Member Author

tlambert03 commented Oct 10, 2023

@larsoner, i know you haven't dug too deeply into how things are structured here, but if you have a moment to look at the general protocol, and backend implementations here (ipynb, qt), I'd welcome your input.

I implemented a toolbar as a top level widget (magicgui.widgets.Toolbar()) rather than a method on something like main window or layout. So it would be used by adding/inserting a Toolbar() into a widgets.Container

still need to write tests, and as mentioned above, still contemplating how to add icons (see related #598)

@larsoner
Copy link

So far so good! To get a "native feel" I have generically:

toolbar = widgets.ToolBar()
noop = lambda *args, **kwargs: None
toolbar.add_button("Play", icon="play", callback=noop)
toolbar.add_button("Pause", icon="pause", callback=noop)
toolbar.add_button("Screenshot", icon="camera", callback=noop)
toolbar.add_button("Movie", icon="film", callback=noop)

then in my Qt half doing:

    window = widgets.MainWindow(widgets=columns, layout="horizontal", labels=False)
    window.native.addToolBar(toolbar.native)

and in the NB half doing:

    from ipywidgets import AppLayout
    window = AppLayout(
        header=toolbar.native,
        left_sidebar=columns[0].native,
        center=columns[1].native,
        pane_heights=['30px', 5, 1]
    )

plus the rest of the stuff from mne-tools/mne-python#11990 I get a decent result (other than Qt icons as expected) except for I think the toolbar button text being wrong on Qt?:

NB Qt
Screenshot from 2023-10-10 13-48-51 Screenshot from 2023-10-10 13-47-41

@tlambert03
Copy link
Member Author

thanks!

except for I think the toolbar button text being wrong on Qt?

ah yep, that's related to icons being unfinished. will fix

question for you regarding API:
I'm inclined to add an explicit MainWindow.add_toolbar(tb: widgets.ToolBar) method to take full advantage of QMainWindow.addToolBar. Do you prefer that to a magic alternative where we call that under the hood in the qt backend if a ToolBar is in the list of the widgets? (or perhaps just the first item in the list)?

@larsoner
Copy link

I think the explicit one is better and what I would expect.

@tlambert03
Copy link
Member Author

i'm going to merge this and continue work in #604

@tlambert03 tlambert03 merged commit 16b3658 into pyapp-kit:main Oct 22, 2023
33 of 34 checks passed
@tlambert03 tlambert03 deleted the toolbars branch October 22, 2023 17:39
@tlambert03 tlambert03 changed the title feat: add toolbars [wip] feat: add toolbar widget Oct 22, 2023
@tlambert03 tlambert03 added the enhancement New feature or request label Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for toolbars
2 participants