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

Enhanced the output viewer to display html outputs (in windows machines), improve viewing of graphs and text outputs and improve processing of objects #7853

Merged
merged 26 commits into from Nov 24, 2022

Conversation

Patowhiz
Copy link
Contributor

@Patowhiz Patowhiz commented Sep 7, 2022

This PR replaces PR #7782. It aims to omit the CEF packages from GitHub.
@lloyddewit I managed to omit the CEF packages from GitHub.
I did this by copying changes manually because of the urgency.
The code changes reviewed in PR #7782 are still the same.
@africanmathsinitiative/developers this is ready for review.
Fixes #7121
Fixes #7808
Fixes #7778
Fixes partly #7727

Follow up issues have been outlined in issue #7852.

@lloyddewit
Copy link
Contributor

@Patowhiz Congratulations on getting rid of the 700+ CEF files.
I checked that I didn't have the CEF packages installed and opened the PR branch. I expected that I'd have to install the CEF NuGet packages manually but I attempted a build anyway. I got the following messages:

image

I closed and reopened Visual Studio and did a rebuild solution. I got the following warnings:

image

I checked my NuGet packages and Visual Studio had automatically installed the CEF packages:

image

These were older versions so I upgraded 'CefSharp.WinForms' to the latest version. This automatically upgraded the 3 dependency packages:

image

But I still got the same three warnings. Do you get these warnings also? Can we remove them?

I will now start the code peer review.

@Patowhiz
Copy link
Contributor Author

Patowhiz commented Sep 8, 2022

@lloyddewit I think that's just because the dll targets an X86 CPU architecture. That's what the message says.
CefSharp typically comes with both X64 and X86 dlls. Then during runtime, loads the appropriate dll by detecting the set architecture from the program assembly file.

@lloyddewit
Copy link
Contributor

@lloyddewit I think that's just because the dll targets an X86 CPU architecture. That's what the message says. CefSharp typically comes with both X64 and X86 dlls. Then during runtime, loads the appropriate dll by detecting the set architecture from the program assembly file.

yes, I think you're correct - thank you

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 large and significant PR.
I only have fairly minor comments

.gitignore Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
instat/Model/Output/clsOutputElement.vb Outdated Show resolved Hide resolved
instat/Model/Output/clsOutputElement.vb Outdated Show resolved Hide resolved
instat/Model/Output/clsOutputElement.vb Show resolved Hide resolved
instat/frmMaximiseOutput.vb Outdated Show resolved Hide resolved
instat/ucrInputComboBox.vb Outdated Show resolved Hide resolved
instat/ucrInputComboBox.vb Outdated Show resolved Hide resolved
instat/ucrInputComboBox.vb Outdated Show resolved Hide resolved
instat/ucrSave.vb Outdated Show resolved Hide resolved
@Patowhiz
Copy link
Contributor Author

Patowhiz commented Oct 3, 2022

@rdstern @lloyddewit this is now ready for you.

@rdstern As discussed I've added the new implementation of setting the heights of the outputs. The setting is in the options dialog. The English description of this new setting is not available because it is being translated. I think the translation bit can be done in a separate PR. I've set maximum output heights to be 300 by default.

Also not, objects are not being displayed on the view objects dialog. I noticed I have to make changes on different controls for this. I think it's best to do that in a different PR, I now have this task as my top priority.

@lloyddewit
Copy link
Contributor

@Patowhiz there are 15-20 open comments above. If you resolve them and let me know when done, then I'll review this PR again.
When you resolve the comments, please do not click 'Resolve Conversation' (otherwise I cannot keep track of which comments I've reviewed). I will click 'Resolve Conversation' next time I review.
Thanks!

@Patowhiz
Copy link
Contributor Author

@lloyddewit I have addressed all the comments.
@rdstern kindly retest this.
Before testing please note the following;

Dialogs that demonstrate the new changes

  1. Prepare > Check Data > One Variable Summaries. Demonstrates how text and table outputs are displayed.
  2. Prepare > Check Data > View/Delete Labels.
  3. Describe > Two/Three variables > Pivot Table. Demonstrates complex html outputs.
  4. Describe > Specific Tables/Graphs > Frequency / Summary Tables.
  5. Describe > Specific Tables/Graphs > Point (Scatter) Plot.

Dialogs that need discussions before refactoring

  1. Describe > Two/Three variables > Summarise

Dialogs that can be refactored by the team to use the new implementation

  1. Describe > Two/Three variables > Two-Way Frequencies
  2. Describe > Two/Three variables > Three-Way frequencies
  3. All dialogs that have a save control

Issues known

  1. Every time a html output is produced, the output view adds the web control which initiates a new browser process similar to the way a new tab is created in Chrome. This means more memory usage. There may be need to prioritise how the output viewer can reclaim back that memory when the user is no longer viewing the html output..
  2. Currently viewing of html objects that produce 2 html files is not supported. The R function added only supports viewing of a single html file.
  3. Plotly html objects may produce html files that don't return the correct CSS height. I'll have to investigate a work around.
  4. Some dialogs may take some time to produce the html objects. This is not related to the new control that only reads from the produced html file dynamically.
  5. At times the output window does not scroll smoothly. This is an existing issue.
  6. Viewing of objects dialog no longer works. Intentional changes to R-Instat object metadata is the cause.
  7. Backward incompatibility of rds files saved by previous R-Instat versions. This is only an issue for the very very old versions. Saving of objects was already disabled in many previous versions.

The above issues and others have been outlined in issue #7852 as follow up issues.

New enhancements and concepts introduced by this PR

  1. Viewing of html objects in R-Instat.
  2. Retrieval of R outputs into the .Net front end.
  3. Maximising of outputs in the output window.
  4. Setting the maximum heights of outputs in the output window.
  5. New way of processing and managing objects.
  6. New object types. All outputs that go the output window can now be saved as objects. @rdstern this is very key and may affect how we conceptualise a dialog that has an output.
  7. Lot's of code enhancements both at R and .Net level.

@rdstern the changes made in this PR affect almost all dialogs in the background. We are aiming for another release before December. I suggest while we wait for @lloyddewit code review, you do all your testing through changes in this PR. I have updated it with the latest changes from the master. I'm keen on identifying any regression I may have introduced early enough.

Thanks.

rdstern
rdstern previously approved these changes Nov 11, 2022
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 looks great.
I tried all the dialogues you said have changed. They all seem ok to me, though I couldn't see what was different in the scatterplot (which you said was changed) and another graph. I tried the bar chart and it seems to behave similarly. I like that you have still left the R-viewer available, and also tried plotly.

You mention that the Describe > 2/3 variable > Summarise still needs discussion. Just to add that this dialogue also has a pull request, (#7920) so perhaps that should be merged first. Vitalis is doing that. Or perhaps he could include the changes for the new output within it?

Can this be merged now if @lloyddewit is also happy, or do things on objects need to change first?

@Patowhiz
Copy link
Contributor Author

@rdstern I'd suggest PR #7920 by @Vitalis95 to wait . The changes made here are meant to demonstrate how dialogs should use the save control.
The scatter plot dialog has a similar change as an example for other graph dialogs.
Of course, I had to support existing dialogs to prevent regression.

@rdstern are you happy with the height set for the outputs, they have fixed heights.

We now have a control for editing and viewing scripts. Should I use the control in the output viewer as well?

@ChrisMarsh82
Copy link
Contributor

@Patowhiz There are 3 conflicts that need to be resolved. Hopefully when they are resolved we should be able to merge this change

@Patowhiz
Copy link
Contributor Author

@ChrisMarsh82 @lloyddewit I have resolved the conflicts. Would be good to have this merged soon, so that @rdstern can keep testing anything broken before our next release.
Changes made here affect so many dialogs.

@rdstern
Copy link
Collaborator

rdstern commented Nov 22, 2022

@Patowhiz I comment as I try different things.
I started with an exercise from Tutorial 1 - so the diamonds dataset from ggplot2 in the library.
Using Describe > 2/3 variables > Graph
image
Here is the output window:
image

You only see half the graph. I originally thought it wasn't scrolling to the bottom, but the half is all there is. You can maximise and that works well.
But in the output window you only have half the graph as shown more clearly below.
image

I am now trying more things!
My next was a table. This was one that used to use a browser. It is all in black!
image

The maximise is fine though.

The second table was also fine - here it is:
image

It initially looks cut-off like the graph above really is, but you can scroll down to the bottom of the table. So this is great.

Finally, so far, I note that saving graphs now doesn't seem to save anything. It used to save the graphs for the session and then they disappeared. Now they are currently not saved. I believe @Patowhiz knows about this. Whether the object stuff will get into the next release, is not so clear. Higher priority is the smaller improvements so all dialogues that use a browser (e.g. one variable frequency) can now use the output window.

@rdstern
Copy link
Collaborator

rdstern commented Nov 23, 2022

@Patowhiz and @ChrisMarsh82 I note that none of the points above prevent the new system being used. So I wonder about even merging this pull request in its current form?
a) Then there are a few more additions needed, that could proceed now - particularly with other dialogues that currently use a browser. They could be adapted this week and be done mainly by @N-thony and the team.
b) This could be in parallel with further tweaks to this facility.

I remain very keen to have a new version (0.7.8?) in time for an important climatic workshop that starts on 5th December in Reading. But it is for a group who would be ok (even happy) to be the first on a provisional release. So it needs to be downloadable from slack, but not necessarily from the website.

@Patowhiz
Copy link
Contributor Author

@rdstern thanks for going through the PR.

Interesting to see why the graph produced is not scrollable. I'll check on that, I'm keen also to find out of there is other graphs produced that are not scrollable.

Remember, we deliberately decided to limit the size of the outputs in the output window, reason being, at times we have very long outputs and they become problematic when scrolling, we didn't have a work around until now that we have the new maximise feature.

I'm aware of the blank black. It relates to rendering in windows 11 machines that have high resolutions, it's not present in windows 10 machine. I'm already thinking about this.

@lloyddewit
Copy link
Contributor

@Patowhiz Thank you for all the changes. I resolved all the open comments apart from 5. Please see above.

I also have many compilation errors (see example below). It seems to be related to VB and WinForms classes. Did something change in the project configuration? If you don't have this problem, then should we ask another developer to test in case the problem is with my system?

If we can resolve the above, then I'm happy to approve.

Thanks

image

@Patowhiz
Copy link
Contributor Author

@lloyddewit this is odd. It compiles and runs fine in 2 machines. @rdstern did you get the same compilation errors, I presume you were able to test because it compiled fine on your end.
@N-thony could you try testing this PR in your machine

@rdstern
Copy link
Collaborator

rdstern commented Nov 23, 2022

@Patowhiz and @lloyddewit it ran on my machine. And I am usually the odd one out here - that it doesn't run on my machine, but is ok on many others.
By the way you mentioned (I think) that some of my problems above were ok in Windows 10. I am still using Windows 10. I haven't changed to Windows 11 yet.

@N-thony
Copy link
Collaborator

N-thony commented Nov 23, 2022

@lloyddewit this is odd. It compiles and runs fine in 2 machines. @rdstern did you get the same compilation errors, I presume you were able to test because it compiled fine on your end. @N-thony could you try testing this PR in your machine

@Patowhiz @lloyddewit I have tried with my machine, this is what I got
image

@N-thony
Copy link
Collaborator

N-thony commented Nov 23, 2022

@Patowhiz I have tried the Describe > 2/3 Variables > 3-way frequencies dialogue and it takes me to the browser, is there something I missed?
image

I also tried the Climatic > Check Data > Inventory plot dialogue I get the following error.
image

@Patowhiz
Copy link
Contributor Author

@rdstern Describe > 2/3 Variables > 3-way frequencies dialog will have to be refactored to use the "new system". That can be done by the team. As explained in my comments above and in issue #7852 any dialog that produces a html output will have to use the "new system"

@N-thony Climatic > Check Data > Inventory plot dialog is implemented incorrected. I think the R function used as a before code should be used as the base code. It currently has no base function. That's why @rdstern got the error above, which I have now made sure that dialogs implemented in such incorrect ways won't raise that error. I still suggest the dialog to be correctly implemented. Perhaps @anastasia-mbithe could do it or any other team member you suggest. Thanks

@Patowhiz
Copy link
Contributor Author

@N-thony thanks for testing whether this PR compiles.
@lloyddewit my machine, @rdstern and @N-thony are able to compile the changes here. The Github build checks are also successful.
I wonder what could be happening in yours.

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 and @lloyddewit I am still getting the same features as reported earlier. But no problem compiling etc.
I am approving, as I can live with those problems for now, and it will be good for the team to work on the remaining dialogues that currently give html output in a browser.

@lloyddewit lloyddewit changed the title Enhances the output viewer to display html outputs (in windows machines), improves viewing of graphs and text outputs and improves processing of objects Enhanced the output viewer to display html outputs (in windows machines), improve viewing of graphs and text outputs and improve processing of objects Nov 24, 2022
@lloyddewit
Copy link
Contributor

@Patowhiz Regarding my compilation errors, I deleted the PR branch, made a new PR branch and did a 'rebuild solution'. It automatically downloaded the CefSharp library, fixed the compilation errors and built successfully.
Happy to merge.
Thank you for this big piece of work!

@lloyddewit lloyddewit merged commit bcd8378 into IDEMSInternational:master Nov 24, 2022
@Patowhiz
Copy link
Contributor Author

Thanks @lloyddewit.

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