REF: button bar rework - template switch menu + Jira reporting button #572

klauer · 2023-08-28T20:58:53Z

Description

Make template switch menu a bit more intuitive by listing classes and their corresponding displays to choose
Add a "Jira report button" to the top level
Remove buttons for swapping directly to different template types

Motivation and Context

Template selection is confusing
Closes Typhos Jira report widget unused for over 2 years #569 (or tries to address it by making Jira a separate tool button)
Closes Disambiguate duplicate template filenames #524

How Has This Been Tested?

Interactively
Jira reporting still works

Where Has This Been Documented?

PR text

Screenshots (if appropriate):

The new toolbar:

! being "create jira report":
->
... shows all templates from all categories:

klauer · 2023-08-28T21:04:14Z

Alternatively, a bit more drastic of a change:

I'd kinda like to get rid of the buttons that were once described as "too scary to click on" in user feedback

ZLLentz · 2023-08-29T23:14:42Z

I think the fewer-buttons look is pretty nice fwiw

tangkong · 2023-08-31T16:11:21Z

I think this looks pretty great, much simpler. One thought I had was maybe we could let the user know which template they're using. I'm not sure how easy that would be (as in I don't know how I'd implement it off hand), but I'm envisioning something like this:

Otherwise the user might one without knowing how to get back to the template they were using

tangkong · 2023-08-31T17:31:11Z

A sort-of minimal implementation of my suggestion. We should of course set the stylesheet outside this callback but the idea remains:

def add_template(filename: pathlib.Path) -> None:
    def switch_template(*, filename: pathlib.Path = filename):
        self.force_template = filename

     action = base_menu.addAction(str(filename))
     action.triggered.connect(switch_template)
     if self.current_template == filename:
         base_menu.setDefaultAction(action)
     base_menu.setStyleSheet("QMenu::item:default { color: #ff0000; }")
     actions.append(action)

@tangkong

Kudos to @tangkong for the suggestion + implementation

klauer · 2023-08-31T19:09:07Z

Great suggestion @tangkong - pushed a commit with this:

(Styling may need some work)

…_menu

klauer · 2023-09-01T19:14:55Z

typhos/display.py

Thoughts about this ordering of template types + instead?

I think it's nice! It helps make the template organization/hierarchy a little more obvious.

Leads me to wonder if people would ever want the base templates. I'm sure there's a world where we do, but for now people probably don't care huh.

Yeah, there probably is little reason to display those - though maybe better to display more options than less just in case? The menu should look less cluttered for a non-development checkout version, but not much better admittedly.

Another thought is that this menu could also have nested submenus. I'm fond of seeing all the choices in a big list personally. That said, base/"advanced" ones still could be in an "Other screens" menu, maybe...

Quick note: the most up-to-date version of this looks even better with the shared prefixes cleaned up
I'm curious how this will end up looking for dev_conda

tangkong

I think this looks good, we could quibble about the exact format of template menu but I think it's a clear improvement in its current state

ZLLentz

Nothing to suggest about the code- just two unnecessary observations to distract you.

I'll run with the branch and take a look at it now

ZLLentz · 2023-09-07T16:35:58Z

typhos/display.py

+            self.embedded_screen: "Embedded",
+            self.detailed_screen: "Detailed",
+            self.engineering_screen: "Engineering",
+        }[self]


It's wild to me that this works, the code reads pretty confusingly if you forget that this is an IntEnum (as I did, on initial review).

I don't think many other classes would be able to look themselves up in a dictionary of their own attributes.

I've used this pattern before with enums and kinda liked it - but if you think it's confusing it's worth redoing honestly as "clever code" is not good code

I don't necessarily think it's worth changes- the only reason I was confused is because:

The diff cut off at a point where I didn't see "IntEnum"

I briefly forgot that IntEnum can be used as an int

ZLLentz · 2023-09-07T16:47:04Z

typhos/display.py

+        for template in DEFAULT_TEMPLATES_FLATTEN:
+            add_template(template)
+
+        prefix = os.path.commonprefix(


Why is this widely useful utility function hiding in the os.path module? Python weird

I couldn't agree more...

ZLLentz · 2023-09-07T17:01:17Z

typhos/display.py

Maybe unrelated to this PR and more as a consequence of #563: there's some edge cases now where you can accidentally pick a template that doesn't itself contain a button title bar, and there's no way to go back. Not sure how to handle this case.

That's a good one... maybe the suite needs to be smarter and insert a title bar if it doesn't exist in the newly-created display?

Yeah, either a smart title bar or some alternate way to switch templates- right clicking?
Or, prevent us from swapping to the "dead end" templates I guess (somehow)

ZLLentz

I'm 👍 on all the changes introduced here

REF: template switch menu + Jira reporting button

cd31851

tangkong mentioned this pull request Aug 28, 2023

ENH: layout changes for compact complex screens + row positioner widget #563

Merged

4 tasks

REF: remove display switcher buttons because we all prefer it

6e2fe4d

klauer requested review from ZLLentz and tangkong August 31, 2023 01:08

klauer marked this pull request as ready for review August 31, 2023 01:08

ENH: mark the current template as default + color it

084d70d

Kudos to @tangkong for the suggestion + implementation

klauer added 2 commits September 1, 2023 10:45

Merge remote-tracking branch 'origin/master' into ref_template_switch…

d1bb30f

…_menu

REF: adjust template menu sections + ordering

2da4d4b

klauer commented Sep 1, 2023

View reviewed changes

tangkong approved these changes Sep 5, 2023

View reviewed changes

klauer mentioned this pull request Sep 5, 2023

Disambiguate duplicate template filenames #546

Closed

ZLLentz reviewed Sep 7, 2023

View reviewed changes

ZLLentz approved these changes Sep 7, 2023

View reviewed changes

klauer merged commit 811cd87 into pcdshub:master Sep 8, 2023
7 of 9 checks passed

klauer deleted the ref_template_switch_menu branch September 8, 2023 16:18

klauer mentioned this pull request Sep 8, 2023

Switching to templates without title bars #580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF: button bar rework - template switch menu + Jira reporting button #572

REF: button bar rework - template switch menu + Jira reporting button #572

klauer commented Aug 28, 2023 •

edited

Loading

klauer commented Aug 28, 2023

ZLLentz commented Aug 29, 2023

tangkong commented Aug 31, 2023

tangkong commented Aug 31, 2023 •

edited

Loading

klauer commented Aug 31, 2023

klauer Sep 1, 2023

tangkong Sep 1, 2023

klauer Sep 1, 2023

ZLLentz Sep 7, 2023

tangkong left a comment

ZLLentz left a comment

ZLLentz Sep 7, 2023

klauer Sep 7, 2023

ZLLentz Sep 7, 2023

ZLLentz Sep 7, 2023

klauer Sep 7, 2023

ZLLentz Sep 7, 2023

klauer Sep 7, 2023

ZLLentz Sep 7, 2023

ZLLentz left a comment

REF: button bar rework - template switch menu + Jira reporting button #572

REF: button bar rework - template switch menu + Jira reporting button #572

Conversation

klauer commented Aug 28, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots (if appropriate):

klauer commented Aug 28, 2023

ZLLentz commented Aug 29, 2023

tangkong commented Aug 31, 2023

tangkong commented Aug 31, 2023 • edited Loading

klauer commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangkong left a comment

Choose a reason for hiding this comment

ZLLentz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz left a comment

Choose a reason for hiding this comment

klauer commented Aug 28, 2023 •

edited

Loading

tangkong commented Aug 31, 2023 •

edited

Loading