Proposal: Enhance Button2D

[Nibba2018]

Proposal:

Improve Button2D to accept texts and also include a background.
Also adds few quality of life changes like feedback when the button is clicked etc.

This is also a requirement to implement FileDialog2D as mentioned here.

The previous Button2D class is renamed to Icon2D ToolButton2D.

Feedback appreciated.

Quick Test:

from fury import window, ui
from fury.data import read_viz_icons

sm = window.ShowManager(size=(1280, 720))
button = ui.Button2D(icon_fnames=[("square", read_viz_icons(fname="stop2.png"))],
                     label='Click Me!', size=(100, 20), position=(100, 100))
sm.scene.background((0, 1, 0))
def toggle_label(button):
    button.label = "Clicked!" if button.label == "Click Me!" else "Click Me!"
button.on_click = toggle_label
sm.scene.add(button)
sm.start()

[m-agour]

Hello @Nibba2018,
Thanks for this PR, the new button looks awesome and is more responsive than the old one.

I have some suggestions though

• I noticed that there is a problem with the aspect ratio of the icon, I think it should be preserved.

  

• The old Button2D can be considered a special case of this new Button2D when no label is set. instead of separating them.

• Also it would be amazing to be able to set on_press_bg_color and bg_color.

[Nibba2018]

Thank you for your feedback @m-agour . Will consider your points for my upcoming commits. 👍

I noticed that there is a problem with the aspect ratio of the icon, I think it should be preserved

What would you think would be a better option? Always maintain the aspect ratio or allow the user to choose?

The old Button2D can be considered a special case of this new Button2D when no label is set. instead of separating them.

I was also thinking about the design for this and it surely requires some discussion.

The existing Button2D acts more like a traditional tool button that you would find in modern UI frameworks such as Qt/Gtk. But again it lacks the full functionality of one such as labels, shortcuts etc.

For me the existing Button2D was more like an Advanced form of an Icon which handles multiple icon states by itself.

But again the exiting one can be considered a Base Button and the new one could be renamed as a PushButton. Idk.

Also it would be amazing to be able to set on_press_bg_color and bg_color.

Yep, on it's way.

[m-agour]

Thank you for your feedback @m-agour . Will consider your points for my upcoming commits. 👍

You are welcome, thanks.

I noticed that there is a problem with the aspect ratio of the icon, I think it should be preserved

What would you think would be a better option? Always maintain the aspect ratio or allow the user to choose?

For me, I would always keep the aspect ratio as it is, but I'm not sure about other users.

The old Button2D can be considered a special case of this new Button2D when no label is set. instead of separating them.

I was also thinking about the design for this and it surely requires some discussion.

The existing Button2D acts more like a traditional tool button that you would find in modern UI frameworks such as Qt/Gtk. But again it lacks the full functionality of one such as labels, shortcuts etc.

For me the existing Button2D was more like an Advanced form of an Icon which handles multiple icon states by itself.

But again the exiting one can be considered a Base Button and the new one could be renamed as a PushButton. Idk.

Yes you are right, maybe it should be this way as you did, but the name Icon2D would be confusing in the tutorials, so maybe the new button should be used instead of the old one. I would say if the new button can only have icons without a label, maybe The old one should have the same API as the old one and the same defaults, then the old one could be removed.
This is because there are some people who might be using the old button in their code.
Also @skoudoro's opinion is needed in this point
.

[Nibba2018]

Yes you are right, maybe it should be this way as you did, but the name Icon2D would be confusing in the tutorials, so maybe the new button should be used instead of the old one. I would say if the new button can only have icons without a label, maybe The old one should have the same API as the old one and the same defaults, then the old one could be removed.

Yes indeed, Icon2D is not the best name. I just named it differently to avoid confusion. Maybe I should add Proposal to the title to clear any confusion. 👍

[skoudoro]

For me, I would always keep the aspect ratio as it is, but I'm not sure about other users.

Same here, I would always keep the aspect ratio. It simplifies the life of many users. We can allow the user to choose for advanced users but by default, always keep the aspect ratio.

The existing Button2D acts more like a traditional tool button

I like the name ToolButton, You can rename it like this for now (since I do not like Icon2D). We might need a deprecation cycle.

In any case, we should unify the codebase of both buttons to simplify the maintenance. Actually, I wonder why you had to create a new class instead of updating the current one. What are the incompatibilities?

[codecov]

Codecov Report

Merging #636 (ccfd72e) into master (054d709) will decrease coverage by 0.23%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   50.41%   50.17%   -0.24%     
==========================================
  Files         120      120              
  Lines       27160    27354     +194     
  Branches     3001     3011      +10     
==========================================
+ Hits        13693    13726      +33     
- Misses      13006    13164     +158     
- Partials      461      464       +3     

Impacted Files	Coverage Δ
fury/ui/core.py	0.00% <0.00%> (ø)
fury/ui/elements.py	0.00% <0.00%> (ø)
fury/ui/tests/test_core.py	0.00% <0.00%> (ø)
fury/fury/ui/core.py	84.37% <0.00%> (-9.02%)	⬇️
fury/fury/stream/tools.py	90.94% <0.00%> (-0.22%)	⬇️
fury/fury/ui/elements.py	90.01% <0.00%> (-0.01%)	⬇️

[Nibba2018]

Hi @skoudoro

My motivation behind creating a new class was to separate the concerns among them.
For e.g The Old_Button2D is used as a tool button for our other UI components for e.g the arrow button in combobox and the buttons of dashboard panel of DrawPanel2D.
The main functionality required by the button is to switch between icons and trigger a callback for the respective Composite UI which is very well served by our Old_Button2D.

The New implementation on the other hand requires a background and a text actor which might not be necessary for our previous UI components leading to bloating.

We might consider the old button to be a special case of our new button as mentioned by @m-agour by hiding the background and text actor when there aren't any text but then the hidden actors will be part of the object and therefore would increase bloat too. This may also lead to further inconsistencies in the future for different applications of the button.

Regarding maintainability, I think our old implementation is complete enough to act as a ToolButton and we can instead focus on adding additional features to our new one.

I would Like to know your thoughts on this thanks!