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

Initial implementation of the undelete mechanism for fitting tabs. #2778

Closed
wants to merge 417 commits into from

Conversation

rozyczko
Copy link
Member

Description

User request addressed - it is easy (too easy) to delete a fitting tab. It would be good to be able to recover it somehow.

This PR is a cruft before the proposed Undo stack is implemented, allowing for more streamlined action of recovering UI errors.

Here, we just store the deleted tab and if needed, populate the perspective with it.
The key shortcut to restore the deleted tab is Ctrl-T, akin to the web browser standard.

How Has This Been Tested?

Local testing on win10

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)

@rozyczko rozyczko marked this pull request as draft January 21, 2024 20:41
@rozyczko rozyczko requested a review from gonzalezma January 22, 2024 14:38
@gonzalezma
Copy link
Contributor

gonzalezma commented Jan 22, 2024

Tested functionality and works as intended, the last FitPage deleted is recovered. Some remarks:

  1. The recovered FitPage gets a new FitPage number. It would be nice (if possible) to be able to recover the FitPage with the same number as it had (see also next point).
  2. If one deletes a FitPage, then creates a new one by sending some data to fitting, and then do Ctrl+Tab, one gets two FitPages with the same number, which I guess will cause problems.
  3. When the Constrained or Simultaneous Fit is open, the recovered FitPage gets the number 302.
  4. The removed FitPage disappears from the Simultaneous Fit tab and the recovered page cannot be added. Even after closing and opening this tab, the recovered FitPage will not appear there.
  5. Sometimes the deleted page is not recovered. It is hard to determine the exact circumstances, but the following seems to fail consistently: a) Load the 4 x_coreshell_#.txt data files. b) Send them to Fitting. c) Select model onion. d) Delete one page and try to recover it. e) It fails. But with the sphere model seems to work (could it be related to the multiplicity?)

@rozyczko rozyczko marked this pull request as ready for review January 25, 2024 08:44
@butlerpd
Copy link
Member

Given this is an intermediate quick fix, I'm guessing this should be included in 6.0.0 beta?

@wpotrzebowski
Copy link
Contributor

Maybe I am not testing it properly but it seems to work fine before I start fitting (I can undo delete). However, as soon as I run the fit I cannot do that for any page. BTW, on MAC is Cmd + T, which we will need to document.

@gonzalezma
Copy link
Contributor

Tested functionality (on Windows 10) and almost everything worked well, including the issue reported by @wpotrzebowski. I loaded several FitPages, made a fit and then deleted one and I could restore it using Ctr+T.
The only problem that I could reproduce is the one with the onion model. If I send a data set to Fitting, select the onion model and then remove the page before doing anything else I cannot restore it. However, as soon as there is at least one parameter selected to be fitted the page can be restored, so for practical purposes I would say this is working as expected and can be merged.

@rozyczko
Copy link
Member Author

rozyczko commented Apr 8, 2024

Tested functionality (on Windows 10) and almost everything worked well, including the issue reported by @wpotrzebowski. I loaded several FitPages, made a fit and then deleted one and I could restore it using Ctr+T. The only problem that I could reproduce is the one with the onion model. If I send a data set to Fitting, select the onion model and then remove the page before doing anything else I cannot restore it. However, as soon as there is at least one parameter selected to be fitted the page can be restored, so for practical purposes I would say this is working as expected and can be merged.

Good observation, which led me to a tab object lifetime issue. This is now solved and the onion model restores just fine.
(apart of the n value, but we knew about this parameter and its lack of correct serialization)

@rozyczko
Copy link
Member Author

Pinging for re-review, please

Copy link
Contributor

@gonzalezma gonzalezma left a comment

Choose a reason for hiding this comment

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

Worked fine in all my tests, including the onion model so I think this can be merged. I think this is a useful intermediate solution to the problem of deleting unintentionally a FitPage (while waiting for a full Undo button), so I agree with @butlerpd suggestion of including this also in release 6.0.0.

@lucas-wilkins
Copy link
Contributor

Shall we merge this? @rozyczko @butlerpd - the only reason haven't clicked the button is because of the 6.0.0 question.

@butlerpd
Copy link
Member

butlerpd commented Jul 3, 2024

Good point Lucas. This has been sitting around for a while and somehow did not get pulled into the beta even though it was approved back in April? If we are going to do a beta 2 anyway I guess my vote would be to merge this into 6.0? That would be in the grey area of violating our rule of no new bug fixes (for prior bugs) but ...? I guess the issue is that it would have to be rebased? ...or cherry picking the 3 commits might be easier?

What do others think/vote?

@smk78
Copy link
Contributor

smk78 commented Jul 3, 2024

I vote merge. But it is against main.

lucas-wilkins and others added 17 commits July 21, 2024 20:54
…meter space to ensure the file is loaded properly
…-tests

2976 auxsas has nondeterminstic tests
…eds-a-help-button-documentation

2905 - Orientation Viewer Tool needs a Help Button & Documentation
…sc-since-506

2831 - fix arrows, stop crashing, fix and improve PDB reading
2981: Eliminate Errors on Density Panel Calculation
klytje and others added 23 commits October 22, 2024 09:24
Added new configuration system change, it's the most significant change I've made.
…ndow, but remove mention of documentation generation
Release 6.0.0: Show 404 message instead of regen
@wpotrzebowski
Copy link
Contributor

The artifacts have expired. Is there a way to rerun jobs with pushing something to the code?

@rozyczko
Copy link
Member Author

new version cherry picked to merge cleanly with main is here: #3140

@krzywon
Copy link
Contributor

krzywon commented Oct 28, 2024

I think we can close this PR now that the other is against main. @rozyczko?

@rozyczko
Copy link
Member Author

I think we can close this PR now that the other is against main. @rozyczko?

We definitely can - I just wanted to make sure we can get to it from the other PR. But we can link to closed PR of course.

@rozyczko rozyczko closed this Oct 28, 2024
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.