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

Adding and setting save controls to dialogs #6434

Merged

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented Apr 29, 2021

Fixes #6397

@lloyddewit For some reasons dlgRecodeNumeric is implemented differently, for instance the setting of R code is not done in the 'normal' way and parameters are also manually written. So I only focused on adding the save control. However, I think it should be redone in 'normal' way because I see no reason as to why it shouldn't, but that can be done after discussions.

I also had to extend the save control to support dialogs with multiple receivers in regards to positioning the new column produced.

I also made major changes to dlgCompare in Climatic > Compare > Calculation to make it use the 2 save controls.

@africanmathsinitiative/developers this is now ready for review.

@Patowhiz Patowhiz marked this pull request as ready for review April 29, 2021 16:04
rdstern
rdstern previously approved these changes Apr 30, 2021
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@lloyddewit this is work by @Patowhiz on many dialogues. It is a difficult one to check completely. It clearly is an improvement and the work will continue with more dialogues and perhaps will also add more small improvements to ones that are "finished". I have not found any problems, so am approving on the grounds it is an improvement. Once merged we will do another round of these dialogues and others will also take over adding these improvements to further dialogues.
That's subject (of course) to the code cxhanges all being sensible.

instat/dlgAnonymiseIDColumn.vb Show resolved Hide resolved
instat/dlgDuplicates.vb Show resolved Hide resolved
instat/dlgRecodeNumeric.vb Outdated Show resolved Hide resolved
@Patowhiz
Copy link
Contributor Author

Patowhiz commented May 6, 2021

@rdstern @lloyddewit if this PR looks good then I suggest merging it. It contains changes to 42 files and this could easily result to conflicts that may need me to redo the work.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented May 6, 2021

All items in the issue have now been fixed. @rdstern @lloyddewit this is ready for your review.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@lloyddewit This has changed many dialogues - I think 42. I have checked quite a few and am happy with what they do. I notice the alignment has been sorted out - minor, but important. And the Position control seems now to have been added systematically.
I still have some questions, some minor e.g. the colon is still missing in the calculator, and many still default to the end, rather than after the column producing the transformation. But I still propose these changes be merged and we then continue to look again at many of the respective dialogues.

@dannyparsons dannyparsons merged commit 8a01de0 into IDEMSInternational:master May 12, 2021
@Patowhiz
Copy link
Contributor Author

@lloyddewit For some reasons dlgRecodeNumeric is implemented differently, for instance the setting of R code is not done in the 'normal' way and parameters are also manually written. So I only focused on adding the save control. However, I think it should be redone in 'normal' way because I see no reason as to why it shouldn't, but that can be done after discussions.

@lloyddewit if you get time and have a look at the dialog and end up agreeing with my above comment, please assign someone to refactor the dialog code and make it consistent with how other dialogs have been implemented. This would be helpful to future developers that may want to look at code implementation examples in the dialogs. Thanks

@lloyddewit
Copy link
Contributor

@lloyddewit For some reasons dlgRecodeNumeric is implemented differently, for instance the setting of R code is not done in the 'normal' way and parameters are also manually written. So I only focused on adding the save control. However, I think it should be redone in 'normal' way because I see no reason as to why it shouldn't, but that can be done after discussions.

@lloyddewit if you get time and have a look at the dialog and end up agreeing with my above comment, please assign someone to refactor the dialog code and make it consistent with how other dialogs have been implemented. This would be helpful to future developers that may want to look at code implementation examples in the dialogs. Thanks

@Patowhiz Thanks for the suggestion.
@Ivanluv In PR #6407 you are trying to open different dialogs from the script. In order to estimate the amount of effort to implement the 'open from script' functionality for all R-Instat dialogs, you are interested in finding dialogs that work in sub-standard ways. Would it be interesting for you to look at 'dlgRecodeNumeric' to see if it has any special characteristics that would make 'open from script' difficult?

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

Successfully merging this pull request may close these issues.

Completing the Position control for saving new variables
6 participants