ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Add a confirmation dialog when the reference viewer is closed #1066

Closed ejeschke closed 1 year ago

ejeschke commented 1 year ago
ejeschke commented 1 year ago

@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 commented 1 year ago

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

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

ejeschke commented 1 year ago

Rebased to pick up the RTD fix

ejeschke commented 1 year ago

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 commented 1 year ago

Re: https://github.com/ejeschke/ginga/pull/1066#issuecomment-1712146057

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 commented 1 year ago

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 commented 1 year ago

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

ejeschke commented 1 year ago

Thanks, @pllim!