Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Set popup verbosity to warnings #1368

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Jun 2, 2022

Description

This pull request is to stop emitting popup messages unless it is warnings or worse. This is because you can still see the mundane info stuff in the history logger. I have always been annoyed by all the popup messages when I load any of the viz.

With this patch, for instance, you will only see these when you open up the history logger thingy if nothing goes wrong on load.

Screenshot 2022-06-02 125711

This is a direct follow-up of #1352

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • Did the CI pass? If not, are the failures related?
  • Is a change log needed? If yes, is it added to CHANGES.rst?
  • Is a milestone set?
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@pllim pllim added the 💤 enhancement New feature or request label Jun 2, 2022
@pllim pllim added this to the 2.7 milestone Jun 2, 2022
@github-actions github-actions bot added the documentation Explanation of code and concepts label Jun 2, 2022
@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #1368 (4c7afd4) into main (7b5e828) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1368      +/-   ##
==========================================
- Coverage   84.93%   84.87%   -0.07%     
==========================================
  Files          91       91              
  Lines        8295     8295              
==========================================
- Hits         7045     7040       -5     
- Misses       1250     1255       +5     
Impacted Files Coverage Δ
jdaviz/cli.py 43.75% <ø> (ø)
jdaviz/app.py 92.75% <100.00%> (ø)
jdaviz/core/helpers.py 34.95% <100.00%> (ø)
jdaviz/utils.py 84.53% <0.00%> (-5.16%) ⬇️

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 7b5e828...4c7afd4. Read the comment docs.

@kecnry
Copy link
Member

kecnry commented Jun 2, 2022

I'm all in favor of this, as long as the success messages aren't ever relied on to communicate essential information. I think we're slowly getting away from any of those cases (for example: automatically generated data labels from plugins used to only be visible from the snackbar, but now is visible in the plugin with options to either plot or not), but I'm not sure if any are left or not.

With this change, we also can start using success/info level snackbars more often for logging purposes without having to avoid them for annoyance concerns.

@pllim
Copy link
Contributor Author

pllim commented Jun 2, 2022

we also can start using success/info level snackbars more often for logging purposes

Now that you mention this... 'debug' is a valid option but the cake is a lie?

jdaviz/jdaviz/app.py

Lines 310 to 318 in 410361e

# https://material-ui.com/customization/palette/ provides these options:
# success, info, warning, error, secondary, primary
# We have these options:
# debug, info, warning, error
# Therefore:
# * debug is not used, it is more for the future if we also have a logger.
# * info lets everything through
# * success, secondary, and primary are treated as info (not sure what they are used for)
# * None is also treated as info (when color is not set)

@pllim
Copy link
Contributor Author

pllim commented Jun 2, 2022

the success messages aren't ever relied on to communicate essential information

This is a GUI, so if something is successful, it should be obvious enough without the annoying popup? We should not recreate the Clippy experience.

@javerbukh
Copy link
Contributor

I will say in regards to @kecnry's point, the line lists plugin relies on a snackbar message communicating to the user that the lines are hidden by default and need to be manually turned on. Otherwise the user will click "load list" and not see any change. That might just be a flaw with the plugin though.

@pllim
Copy link
Contributor Author

pllim commented Jun 2, 2022

snackbar message communicating to the user that the lines are hidden by default

Should we upgrade to warning?

Another option we can pursue is to enable user configuration of software behavior like matplotlibrc, then I can set mine to warning and stop opening PRs. (Which reminds me, that was one of the requested feature from hack day, so I should open a ticket for it if not one already. See #1371)

@kecnry
Copy link
Member

kecnry commented Jun 2, 2022

the line lists plugin relies on a snackbar message communicating to the user that the lines are hidden by default and need to be manually turned on

I think we should address this in the plugin itself (and maybe in the docstring for the API case).

then I can set mine to warning and stop opening PRs

I think even if we add custom settings, etc, it's still useful to change the default as long once we address cases like the line list case @javerbukh brought up.

@pllim
Copy link
Contributor Author

pllim commented Jun 2, 2022

as long once we address cases like the line list case

Should I open a new issue about the line list and have it block this PR then?

@pllim pllim mentioned this pull request Jun 9, 2022
11 tasks
@pllim
Copy link
Contributor Author

pllim commented Jun 9, 2022

Would something like #1388 work for Line Lists?

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

Approving this, although I would like to see #1388 in before this gets merged (technically need one more approval there).

@pllim

This comment was marked as resolved.

@pllim
Copy link
Contributor Author

pllim commented Jun 15, 2022

Since this got 2 approvals and is unblocked. I will merge when CI passes. Thanks for the reviews!

@pllim pllim merged commit 285422d into spacetelescope:main Jun 15, 2022
@pllim pllim deleted the verbosity-warn-default branch June 15, 2022 21:31
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation Explanation of code and concepts Ready for final review 💤 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants