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

Add NavigationToolbar to Figure #55

Closed
tblanke opened this issue Dec 2, 2022 · 22 comments · Fixed by #58
Closed

Add NavigationToolbar to Figure #55

tblanke opened this issue Dec 2, 2022 · 22 comments · Fixed by #58
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@tblanke
Copy link
Collaborator

tblanke commented Dec 2, 2022

Hi @wouterpeere,

I have added a Navigation Toolbar to the figures.
Maybe then the save Figure button is obsolet then.

With best regards
@tblanke

@tblanke tblanke linked a pull request Dec 2, 2022 that will close this issue
@wouterpeere wouterpeere added the enhancement New feature or request label Dec 4, 2022
@wouterpeere wouterpeere added this to the v2.1.1 milestone Dec 4, 2022
@tblanke
Copy link
Collaborator Author

tblanke commented Dec 4, 2022

Hi @wouterpeere ,

the following points have been fixed:

  • The layout of the Figure options isn't so good.
    That has been taken care of by adding the Tab Widget to the Central Widget style sheet.
  • When hovering over the buttons, a rectangle appears
    The tooltip should have been shown there and is now. The Tooltip style has been added to the Central Widget style sheet.
  • When doing a 'optimise load profile' exercise on the gui, the toolbar is shown twice for the first Figure.
    The Navigationtoolbar, canvas, figure and axes creation has been moved to the FigureResults class and the figure has been added as variable in the main class. So that it does not have to be created multiple times.

Can you please have a look at it and check that everthing works?

With best regards
@tblanke

@wouterpeere
Copy link
Owner

Hi @tblanke

Perfect, thanks!
I will take a look at it. If everything seems to work, I will re-open the PR.

Best,
@wouterpeere

@wouterpeere
Copy link
Owner

wouterpeere commented Dec 5, 2022

@tblanke I noticed you created 'fig_temperature' and 'fig_load_duration' variable in main_class.py.
Is this necessary for the NavigationToolbar or is it just a speed improvement so that whenever the result page is opened, the figure does not need to be recalculated/recreated?

In the latter case, I would prefer the figure to be saved in the DataStorage object itself instead of in the borefield object. This simplifies the code in the main class.

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 6, 2022

Hi @wouterpeere ,

That is a good idea. I have made to adaptions for this and pushed in the branch.

With best regards
@tblanke

wouterpeere pushed a commit that referenced this issue Dec 6, 2022
Run display_results each time the result page is shown (solves problem refered to in issue #55)
Restate add_link_2_show
@wouterpeere
Copy link
Owner

Hi @tblanke

This already looks nicer!

I noticed some more strange behaviour:

  • When you have two scenario's (lets say one with aim 1 and one with aim 2) both calculated. When you switch from one scenario to another, the figure on the result page gets bigger each time you switch...
    image
    (The same happens when you keep pressing calculate when the figure is already calculated)

  • [SOLVED] Secondly, when you have calculated a scenario and you change one of the parameters, the result page should be empty, which is not the case. This is not a problem in the main branch at this moment. I solved it by restating that every time the result button is pressed, display_results() should be called. This clears everything from the result page and one can only show new results if the calculation button is pressed.

  • [SOLVED] Thirdly, you removed the add_link_2_show options in the gui_structure for the load duration curve. This however crashes the gui for me whenever I try to open a saved document, due to the fact that, regardless of the aim, it runs the plot_load_duration_curve function (which can only be called in specific conditions). I solved this by restating those two lines.

@wouterpeere
Copy link
Owner

I also made a change to the way the figures are saved in DS. Now, the figures were hardcoded, meaning that every time a new ResultFigure was placed in the gui_structure, this also had to be changed in the DS.
I replaced this by using the list_of_result_figures to automatically create all the attributes for the figures.

@wouterpeere
Copy link
Owner

Some more strange behaviour:

When you load a file that was calculated, the figure displays from the first time (which is perfect), however the labels are not displayed. Is it possible that they aren't saved in the DS?

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 6, 2022

I have fixed the label issue.

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 6, 2022

I can not reproduce the "figure on the result page gets bigger" issue. Can you try to update the packages to the newest version and check if that solves the issue?

@wouterpeere
Copy link
Owner

I can not reproduce the "figure on the result page gets bigger" issue. Can you try to update the packages to the newest version and check if that solves the issue?

Matplotlib and PySide6 are both the latest versions.
To replicate my issue, delete the backupfile so one can start with a 'clean version'. Just press calculate, it will display the temperature profile. If you now press calculate again, it will enlarge the figure.

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 6, 2022

Does it work now?

wouterpeere pushed a commit that referenced this issue Dec 6, 2022
@wouterpeere
Copy link
Owner

@tblanke, no it did not. It caused even some problems while loading.
I reversed those changes and think I solved it by removing the setattr on line 1004. This is unneeded since replace_figure() already sets the self.fig.

Can you confirm that everything is still working by you?

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 6, 2022

Correct.
Yes everthing, work on my side.

@wouterpeere
Copy link
Owner

Perfect!

I will finish my review, check if all the documentation still is correct and I will do the PR.

W.r.t. testing, looking at the coverage results, about 500 lines in combine_gui_window are not tested currently. Do you think it would be possible to increase the test coverage for the gui?

@tblanke tblanke self-assigned this Dec 7, 2022
@tblanke
Copy link
Collaborator Author

tblanke commented Dec 8, 2022

Hi @wouterpeere ,

Great, from my side you can start with the PR.
I will take a look at the testing.

With best regards
@tblanke

@wouterpeere
Copy link
Owner

@tblanke , is there a particular reason why you reset the location of the BACKUP.GHEtool to the default save location? I switched it to another directory because, if one overwrites this backup.ghetool document, it can cause issues.

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 8, 2022

Hi,

I have changed the location, because otherwise the default location is the current working directory. This is an issue for the installed version, because then this location may be in a folder where just admins are allowed to read and write files. I think this is the problem, why the program crashed in the past.
Yes it can also cause issues if the backup file is overwritten but this can be easy fixed by just deleting it. What do you think about creating a hidden file under Documents/GHEtool?

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 8, 2022

We have to create a hidden folder and no hidden file, because the hidden file is causing Permission errors. I have created a new commit for this.

@wouterpeere
Copy link
Owner

Or, instead of creating a hidden folder, we can change the extension for the backup to *.ghetoolbackup, so it won't show up in the dialog box when saving?

@tblanke
Copy link
Collaborator Author

tblanke commented Dec 8, 2022

That is a better idea. I have made the changes.

@wouterpeere
Copy link
Owner

I checked the gui and all looks fine. I've added some more Dutch translations and fixed a small bug w.r.t. the ResultTexts. I will open the PR.

@tblanke will you be able to create some more unit tests for the gui in this issue?

@wouterpeere wouterpeere linked a pull request Dec 9, 2022 that will close this issue
@tblanke
Copy link
Collaborator Author

tblanke commented Dec 11, 2022

Hi @wouterpeere ,

I will create a new branch for the gui testing.

With best regards
@tblanke

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants