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 confirmation dialog when the reference viewer is closed #1066

Merged
merged 2 commits into from
Sep 9, 2023

Conversation

ejeschke
Copy link
Owner

@ejeschke ejeschke commented Sep 8, 2023

  • pops up a confirmation dialog that confirms you intend to quit
  • adds support across backends
  • can be overridden by adding a line to your general.cfg
  • example general.cfg updated

@ejeschke
Copy link
Owner Author

ejeschke commented Sep 8, 2023

@pllim, if/when you have time for a quick review. Just adds a pop-up confirmation dialog when you close the reference viewer.

Not sure why we are getting the RTD build failure, but looks like #1060 is the cause. Did I see something go by in my email about a fix for this in stginga?

@pllim
Copy link
Collaborator

pllim commented Sep 8, 2023

For the docs, hopefully #1067 will fix it. 🤞

Thanks for the ping. I will review when I get the chance.

- pops up a confirmation dialog that confirms you intend to quit
- adds support across backends
- can be overridden by adding a line to your general.cfg
- example general.cfg updated
@ejeschke
Copy link
Owner Author

ejeschke commented Sep 8, 2023

Rebased to pick up the RTD fix

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

As a user, I cannot tell what is the difference between "close" and "shutdown" in Ginga. Can you please clarify?

ginga/gtk3w/Widgets.py Outdated Show resolved Hide resolved
ginga/qtw/Widgets.py Outdated Show resolved Hide resolved
ginga/web/pgw/Widgets.py Outdated Show resolved Hide resolved
@ejeschke
Copy link
Owner Author

ejeschke commented Sep 8, 2023

As a user, I cannot tell what is the difference between "close" and "shutdown" in Ginga. Can you please clarify?

It just provides for a 2-step quit sequence. When someplace calls fv.close() the "close" callback is called. Ginga has a function registered for that callback to pop up a confirmation dialog box. If the user confirms, then fv.quit() is called which initiates the "shutdown" of the program. Any code that as registered for the "shutdown" callback will then get called to properly close any resources and terminate the program.

@pllim
Copy link
Collaborator

pllim commented Sep 8, 2023

Re: #1066 (comment)

Maybe I am not fully understanding but won't it be simpler to have fv.quit(prompt_dialog=True) instead of a separate fv.close()?

@ejeschke
Copy link
Owner Author

ejeschke commented Sep 8, 2023

The "close" and "shutdown" callbacks allow a flexible way to handle the closure of the program. For example, a plugin might want to register for "shutdown" in order to close a running thread they started or close a database connection, or they could register for "close" to make a checkpoint or pop up their own dialog to say that there are unsaved changes, etc. The reference viewer is basically this core around which there are a large collection of plugins.

Plus, we use the framework for many other programs that are not the reference viewer, but benefit from this style.

@pllim
Copy link
Collaborator

pllim commented Sep 8, 2023

Thanks for the clarification! The diff looks reasonable to me and I am okay with this feature's addition. Feel free to merge. 😸

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
@ejeschke ejeschke merged commit 4815b44 into ejeschke:main Sep 9, 2023
7 checks passed
@ejeschke
Copy link
Owner Author

ejeschke commented Sep 9, 2023

Thanks, @pllim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants