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

Improved default output startup commands, comments, heights of outputs and R-Instat initialization #8331

Merged
merged 20 commits into from
May 23, 2023

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented May 16, 2023

Fixes #8091
Fixes #7854
Fixes partly #7979
@africanmathsinitiative/developers this PR has major changes on frmMain. I suggested any other changes to be done on frmMain to wait until this PR is merged. This will help in preventing conflicts.

rdstern
rdstern previously approved these changes May 17, 2023
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.

@Patowhiz this has done a lot. I found only one small detail to change, and would like to see it merged as soon as checked by @lloyddewit

The one very minor suggestion is that, at the start it says:
# Setting display options (e.g Number of significant digits)
That's fine, but it also repeats that, whenever you use the dialogue.
I went in to change the language and it said that again!

The Options Dialog doesn't have the Comments control - perhaps it should? But as a quick change could the Comment for this dialog be simply #Setting Options? (Without changing it on startup, from what it is now.

I did try the output window and it seems much better. I'd like to keep trying it, once it is merged.

That's so minor that I am approving.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented May 17, 2023

@rdstern thanks for testing this even though I'm still making major changes on the different bits.
Should we then just have Setting Options even during startup? in future these options could be more than just display settings.

@rdstern
Copy link
Collaborator

rdstern commented May 17, 2023

@Patowhiz I quite liked the current, more detailed, message on Startup, with the brief message - possibly with access to the Comment control, thereafdter? But, if that takes too much time, I could live with the brief message throughout.

And - assuming @lloyddewit is happy with the code, then I'd be happy to see these changes merged as soon as you are ready, and then another pull request for the further work.

@lloyddewit
Copy link
Contributor

@Patowhiz Is this PR ready for peer review or do you still wish to make some changes? thanks

@Patowhiz
Copy link
Contributor Author

@lloyddewit you can go ahead and review this while I keep making more additions

@lloyddewit
Copy link
Contributor

lloyddewit commented May 19, 2023

@lloyddewit you can go ahead and review this while I keep making more additions

@Patowhiz I think it will be more efficient if I review when the PR is complete, thanks

@rdstern
Copy link
Collaborator

rdstern commented May 19, 2023

@Patowhiz and @lloyddewit I would like to merge what has been done into the next release. It changes the comments and startup, and therefore is captured in screeen shots for our documents and videos. So good to have these changes soon.

@Patowhiz
Copy link
Contributor Author

@lloyddewit @rdstern
Kindly test the following;

  1. Switching on and off comments and commands
  2. Startup comments and commands.
  3. Switching on and off, heights of outputs. Graphs, summaries, tables, HTMLs etc
  4. Auto recovered files. In particular notice how the recovered R script is executed and the differences in the options being reset. @lloyddewit it's still not clear to me why we assume some recovery files could be multiple. I noticed we may have to refactor different bits of recovery implementation.

@rdstern as discussed before, there is much more that I'm adding in regards to the outputs;

  1. Right click menu that has; copy, save, view in plotly, view in R viewer, maximise, view command etc. Each option will depend on the output type.
  2. Making sure that all outputs come in as file. The current exception is the calculator output, so I need to refactor it's code.
  3. Each output should now have it's own custom control. These controls should be enhanced to work consistently with the output page they are contained in, this includes, resizing, saving etc.
  4. Many more.

Considering the urgency of testing this. I can do the above in a separate PR.

@lloyddewit
Copy link
Contributor

@rdstern if you're willing to approve, then I can peer review and we can hopefully merge into the release this week. Thanks

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.

I am approving. However, previously it changed the default Comment text into Dialog: Iy has now returned to the previous setting:

image

I think that's just me, because I have also been using previous versions. @lloyddewit Could you just check this point as you review.

@Patowhiz
Copy link
Contributor Author

@rdstern all previous settings should be retained. Only new users should have Dialog: as the default. Could try and test this by resetting the options to defaults?
After this is merged Dialog: will also need it's own translation.

Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@Patowhiz Thank you for this.
When I reviewed, I noticed lines that were commented out rather than deleted. Is this work in progress?
Also, some of the changes to RLink (also frmMain?) looked like refactoring changes rather than changes directly linked to this PR. Is that correct?

Happy to approve and merge.

@lloyddewit lloyddewit merged commit ae6a99e into IDEMSInternational:master May 23, 2023
@lloyddewit lloyddewit changed the title Changes default output startup commands, comments, heights of outputs and refactors R-Instat initialization implementation Improved default output startup commands, comments, heights of outputs and R-Instat initialization May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants