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

Add a "display_name" option to modify a widget's label (#45) #48

Merged
merged 7 commits into from
Nov 25, 2020
Merged

Add a "display_name" option to modify a widget's label (#45) #48

merged 7 commits into from
Nov 25, 2020

Conversation

HagaiHargil
Copy link
Contributor

@HagaiHargil HagaiHargil commented Nov 22, 2020

Addresses #45.

This PR adds the option to modify a widget's label by including a display_name key in that argument's specific dictionary.

It also adds a paragraph in the docs, does a small change to the examples section that shows this behavior and adds a test.

@codecov
Copy link

codecov bot commented Nov 22, 2020

Codecov Report

Merging #48 (6e523bf) into master (a138582) will decrease coverage by 0.07%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   96.08%   96.01%   -0.08%     
==========================================
  Files          16       16              
  Lines         971      979       +8     
==========================================
+ Hits          933      940       +7     
- Misses         38       39       +1     
Impacted Files Coverage Δ
magicgui/_tests/test_magicgui.py 92.02% <83.33%> (-0.17%) ⬇️
magicgui/core.py 98.83% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a138582...f8c1651. Read the comment docs.

@tlambert03
Copy link
Member

thank you for implementing this @HagaiHargil! and thanks for such a thorough treatment (docs, examples, etc...)!
My only question (and this is for @jni as much as you, since he originally proposed the "display_name" paramater), is whether we might want to name it "label" instead? I can imagine using the same key for label=False to hide a specific label (down the road, when we add that sort of granularity to the labels). so if it's a bool, the label is shown or hidden as indicated, and if it's a string, it's used as the display name. What do you both think?

@GenevieveBuckley
Copy link
Contributor

My only question (and this is for @jni as much as you, since he originally proposed the "display_name" paramater), is whether we might want to name it "label" instead? I can imagine using the same key for label=False to hide a specific label (down the road, when we add that sort of granularity to the labels). so if it's a bool, the label is shown or hidden as indicated, and if it's a string, it's used as the display name. What do you both think?

I feel like:

  • "label" is much more ambiguous than "display_name" (I would probably confuse "label" with the label image layer or something like it if I saw that)
  • I find the idea of cramming multiple functionality into a single kwarg (or dictionary key, in this case) potentially confusing. Would it be simpler to add another dictionary key called something like "hide_display_name" with a boolean value, if you wanted to add this kind of functionality later on?

@tlambert03
Copy link
Member

tlambert03 commented Nov 23, 2020

(I would probably confuse "label" with the label image layer or something like it if I saw that)

i don't feel like there should be any implicit/assumed relationship between magicgui and napari... so that particular concern doesn't resonate with me that much. The word next to the widget is a literal QLabel and "label widgets" are usually read-only text strings, so i feel like "label" is the natural thing to call it here.

I find the idea of cramming multiple functionality into a single kwarg (or dictionary key, in this case) potentially confusing

that I can understand. I kinda liked just saying label=False or label='something different' ... but label='something different' and hide_label=True is totally fine with me too.

@HagaiHargil
Copy link
Contributor Author

I'm 100% behind @tlambert03 on this one. I thought "label" was the better option but went with "display_name" due to it being mentioned in the original issue. I'll happily replace all use cases of one with the other.

My use cases for magicgui are completely independent from napari, so I don't think that there would be any confusion with napari's Label keyword, especially since we're talking about a key within a dictionary within a decorator - all of them are exclusively magicgui's. Moreover, in many common data science APIs there are keywords which behave like @tlambert03 proposed - False or None if you don't want it around, and some value otherwise.

@GenevieveBuckley
Copy link
Contributor

I'm 100% behind @tlambert03 on this one. I thought "label" was the better option but went with "display_name" due to it being mentioned in the original issue. I'll happily replace all use cases of one with the other.

I'm happy to be outvoted. Thanks for making this @HagaiHargil

@tlambert03
Copy link
Member

I'm 100% behind @tlambert03 on this one. I thought "label" was the better option but went with "display_name" due to it being mentioned in the original issue. I'll happily replace all use cases of one with the other.

I'm happy to be outvoted. Thanks for making this @HagaiHargil

@jni, care to weigh in on this and put us back in gridlock? 😂

@jni
Copy link
Contributor

jni commented Nov 24, 2020

@jni, care to weigh in on this and put us back in gridlock? 😂

My reading is that "label" is itself kinda Qt-specific. I would find it confusing. How about just "name"?

@jni
Copy link
Contributor

jni commented Nov 24, 2020

I agree with @GenevieveBuckley about separating the APIs, too. So, name='my widget', display_name=True? I might also suggest that display_name defaults to True if name is given, False otherwise.

@tlambert03
Copy link
Member

tlambert03 commented Nov 24, 2020

@jni, care to weigh in on this and put us back in gridlock? 😂

My reading is that "label" is itself kinda Qt-specific. I would find it confusing. How about just "name"?

I don't think it is... tkinter also uses it, and Wikipedia uses it: https://en.m.wikipedia.org/wiki/Label_(control)

https://anzeljg.github.io/rin2/book2/2405/docs/tkinter/label.html

@jni
Copy link
Contributor

jni commented Nov 24, 2020

Ok fine 😂 I knew as I was writing it that I would regret it. I'll rephrase: I would be confused by the word "label", and I think other people who are not super familiar with GUI frameworks, ie exactly the target audience for magicgui, might also be confused!

I don't feel suuuuper strongly about this, but "name" would easily be my preference.

@tlambert03
Copy link
Member

tlambert03 commented Nov 24, 2020

I think other people who are not super familiar with GUI frameworks, ie exactly the target audience for magicgui, might also be confused!

I don't feel suuuuper strongly about this, but "name" would easily be my preference.

I think "name" is also kinda vague... and I don't think "label" is super jargony. When you want to put a sticker on something to say "what is this thing"... you call it a label. I think the magicgui target audience will figure it out fine. I'd like to stick with label

@tlambert03
Copy link
Member

Thanks @HagaiHargil, we can leave the discussion of hide/show individual labels for later, when we actually support that sort of thing (which probably should wait for #43 anyway).
I appreciate this submission!

@HagaiHargil
Copy link
Contributor Author

HagaiHargil commented Nov 24, 2020

Happy to introduce "label" and thanks for the kind words :)

@tlambert03 tlambert03 merged commit d8a954f into pyapp-kit:master Nov 25, 2020
@tlambert03 tlambert03 mentioned this pull request Dec 22, 2020
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.

4 participants