UI forms as JSON by sjanzou · Pull Request #1338 · NatLabRockies/SAM

sjanzou · 2023-01-30T11:08:06Z

goes with associated pull requests in wex 155 and SAM-private

Add defines to test in ide.cpp and main.cpp

Note that unicode is not currently written to JSON

mjprilliman

I like the json formatting of the UI files. How will we diff callback and equation changes in the future? Seems like it might be more difficult as a single string with \n values.

brtietz · 2023-01-30T18:00:18Z

I like the json formatting of the UI files. How will we diff callback and equation changes in the future? Seems like it might be more difficult as a single string with \n values.

That's a great point. The best recommendation for this seems to be using an array of strings, in which each element of the array is a line: https://stackoverflow.com/a/52557585

If we want to go that route I would recommend completing the review of this format and migrating to that format as a separate issue, so we're a little less locked down on forms changes for the patch.

brtietz

This is really exciting, but I think it needs a little more polish prior to merging. Edit: I was reviewing an older version of the branch - please ignore.

brtietz · 2023-01-30T18:09:28Z

@@ -2348,7 +2347,8 @@ void VarInfo::Write(wxOutputStream &os)
 	wxDataOutputStream out(os);
 	out.Write8(0xe1);
 	//	out.Write8(2);
-	out.Write8(3); // change to version 3 after wxString "UIObject" field added
+//	out.Write8(3); // change to version 3 after wxString "UIObject" field added
+	out.Write8(4); // change to version 3 after wxString "UIObject" field added


Comment doesn't make sense here - what is version 4?

brtietz · 2023-01-30T18:10:38Z

+	// Group
+	Write_JSON_value(doc, "Group", Group);
+	// IndexLabels
+	/* Handle multiline equations in IndexLabels


Nice comment!

brtietz · 2023-01-30T18:11:52Z

 //	sz_form_tools->Add(new wxButton(this, ID_FORM_LOAD_ALL, "Load all", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);
 //	sz_form_tools->Add(new wxButton(this, ID_FORM_SAVE_TEXT, "Save text", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);
 //	sz_form_tools->Add(new wxButton(this, ID_FORM_LOAD_TEXT, "Load text", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);
 //	sz_form_tools->Add(new wxButton(this, ID_FORM_SAVE_ALL_TEXT, "Save all text", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);
-//	sz_form_tools->Add(new wxButton(this, ID_FORM_LOAD_ALL_TEXT, "Load all text", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);
+	//sz_form_tools->Add(new wxButton(this, ID_FORM_LOAD_ALL_TEXT, "Load all text", wxDefaultPosition, wxDefaultSize, wxBU_EXACTFIT), 0, wxALL | wxEXPAND, 2);


Maybe I'm missing something, but why add commented out lines? This is hard to follow with all the commented out code.

Maybe I'm missing something, but why add commented out lines? This is hard to follow with all the commented out code.

The code is temporarily commented so that when merging into develop, the updated text forms can be loaded and then saved as JSON and then the text UI forms can be removed. The forms in develop have diverged from patch so that a separate pull request is necessary for the SAM repo.

mjprilliman · 2023-01-30T18:29:21Z

This is really exciting, but I think it needs a little more polish prior to merging. For example: why are there both "Save" and "Save as JSON" buttons in the IDE?

I feel like there should only be one save button at merge time. Or is there a reason we'll need both for a while?

Why am I not seeing these additional buttons when I build the branches?

brtietz · 2023-01-30T18:39:22Z

Why am I not seeing these additional buttons when I build the branches?

Oh, whoops. I had an older version of SAM 504 checked out.

mjprilliman · 2023-01-30T20:18:40Z

Clicking out of a UI form in the IDE registers a diff in the json without hitting save or making any changes. Diff software cannot find any differences in the files but the file is shown to be modified.

cpaulgilman · 2023-01-30T23:39:23Z

This looks good from the UI.

I also tested loading all configs with this LK script:

techs = list_technologies();
for (t=0;t<#techs;t++)
{
	fins = list_financing(techs[t]);
	for (f=0;f<#fins;f++)
	{
		outln(techs[t] + ', ' + fins[f]);
		configuration(techs[t],fins[f]);
	}	
}

The script works in Windows, but on Linux (Mint 21.1), when I run the script, I get errors like:

Solar Resource Data: "could not find library:SolarResourceData" when loading the Solar Resource Data
Marine Wave Resource Time Series: "Error: 'PacWave East' is not available in the library. Choose a different item."
Wind Resource File: "Error: 'WY Cheyenne_2006' is not available in the library. Choose a different item."

But the forms load fine when opening a case by hand, so not sure it's worth pursuing at this stage.

sjanzou · 2023-01-31T06:55:20Z

Clicking out of a UI form in the IDE registers a diff in the json without hitting save or making any changes. Diff software cannot find any differences in the files but the file is shown to be modified.

Maybe we can write a better code versioning system than github that sometimes defaults to date and time changes instead of actually comparing file contents...

…nfig for JSON forms

sjanzou · 2023-01-31T10:44:49Z

This looks good from the UI.

I also tested loading all configs with this LK script:
techs = list_technologies();
for (t=0;t<#techs;t++)
{
	fins = list_financing(techs[t]);
	for (f=0;f<#fins;f++)
	{
		outln(techs[t] + ', ' + fins[f]);
		configuration(techs[t],fins[f]);
	}	
}
The script works in Windows, but on Linux (Mint 21.1), when I run the script, I get errors like:

Solar Resource Data: "could not find library:SolarResourceData" when loading the Solar Resource Data

Marine Wave Resource Time Series: "Error: 'PacWave East' is not available in the library. Choose a different item."

Wind Resource File: "Error: 'WY Cheyenne_2006' is not available in the library. Choose a different item."

But the forms load fine when opening a case by hand, so not sure it's worth pursuing at this stage.

Where are you running SAM from to get this script error? Did you make from the SAM-private build_linux folder and run from the installation folder? I am not seeing this issue on MacOS or on CentOS7.

brtietz

It looks like the updates to the CI script swapped the target branch from patch to develop, that need to be addressed prior to merge.

Also the comment on line 2351 of variables.cpp is referring to the wrong version number.

Other than that, the new code works well for me - thanks for updating the rapid json environment variable!

…1338

Reset to single line until JSON Array of strings encodes and decodes correctly.

sjanzou · 2023-02-02T11:17:03Z

@brtietz , @mjprilliman , @cpaulgilman the multiline string saving for equations and callbacks has been implemented in this pull request and in wex pull request 155.

An example of changing AA Widgets callback line 36 from x=0; to y=0;

and then using git diff:

Please recheck this pull request along with wex 155 and let me know if you run into any issues.

brtietz · 2023-02-02T17:02:45Z

The diff for changing lines looks great. The diff for adding a new line, however, is quite messy:

Adding one line (13) triggered a change to over 100 lines.

Is there a reason it has to be an object with line numbers as opposed to an array?

sjanzou · 2023-02-02T20:40:27Z

Arrays should work now with the two str replace…. Both objects and arrays had the same escaping sequence issue. I can make the change and test new lines and update the pull request.

…

On Thu, Feb 2, 2023 at 10:02 AM Brian Mirletz ***@***.***> wrote: The diff for changing lines looks great. The diff for adding a new line, however, is quite messy: [image: image] <https://user-images.githubusercontent.com/5530592/216391880-46013a91-cc5b-4c1f-a601-0a2de8f85550.png> Is there a reason it has to be an object with line numbers as opposed to an array? — Reply to this email directly, view it on GitHub <#1338 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRSQBYHDFW4DFLKPI5OVJLWVPSD7ANCNFSM6AAAAAAULAHD64> . You are receiving this because you were assigned.Message ID: ***@***.***>

sjanzou · 2023-02-03T10:16:44Z

After updating to RapidJSON array of strings, single line changes and new line added changes in AA Widgets results in desired diff viewing:

brtietz

This is great! One additional case not shown in Steve's tests, removal:

All of these diffs are perfectly readable, in my opinion.

sjanzou added 22 commits January 15, 2023 04:48

VarInfo updates for saving as JSON

42b8ca6

api updates switching to patch branch base

8a9f3c6

Implement InputPageData Read and Write JSON

1ca904f

Add defines to test in ide.cpp and main.cpp

API updates

a36abdc

Forms as JSON updates to VarDatabase and JSON in wex/utils.cpp

fbe8416

Merge remote-tracking branch 'origin/patch' into SAM_504

1d4eb7b

Merge branch 'ptes-design-pt' into SAM_504

a928e49

remove unused arguments to VarDatabase::Read_JSON

0aba06d

Merge remote-tracking branch 'origin/patch' into SAM_504

36f3172

Writing JSON form but reading is not working correctly

efabe7e

Forms saving as JSON - some errors loading JSON forms

6588c76

Merge remote-tracking branch 'origin/patch' into SAM_504

967d192

latest api updates

1fadbc3

Merge remote-tracking branch 'origin/patch' into SAM_504

07bccc7

merge of patch and api updates

dee1256

Merge remote-tracking branch 'origin/patch' into SAM_504

563f5f3

api updates after latest patch branch updates

286724a

Merge remote-tracking branch 'origin/patch' into SAM_504

40a6211

Updates to get all forms writing and reading

be34296

Note that unicode is not currently written to JSON

All UI forms saving and loading as JSON with Unicode support

9d05b88

unicode support for JSON read and write of equations and callbacks

bc5efbd

All forms converted to JSON for patch branch and UI editor updated

f9de05d

sjanzou self-assigned this Jan 30, 2023

sjanzou requested review from brtietz, cpaulgilman and mjprilliman January 30, 2023 11:08

mjprilliman reviewed Jan 30, 2023

View reviewed changes

brtietz requested changes Jan 30, 2023

View reviewed changes

brtietz self-requested a review January 30, 2023 18:39

sjanzou added 2 commits January 31, 2023 03:13

Update to use RAPIDJSONDIR environment variable and rewrite export_co…

080295f

…nfig for JSON forms

Remove comments and update workflows for RAPIDJSONDIR

e213420

sjanzou requested a review from mjprilliman January 31, 2023 10:42

brtietz requested changes Jan 31, 2023

View reviewed changes

Comment thread .github/workflows/ci.yml Outdated

sjanzou added 3 commits February 1, 2023 02:02

Change ci target branch to patch per pull request 1338

d8926fc

Remove automatic saving when moving through form list per feedback on #…

20469a7

…1338

Multiline equation writing but incorrect json format

4f9992e

Reset to single line until JSON Array of strings encodes and decodes correctly.

brtietz approved these changes Feb 1, 2023

View reviewed changes

sjanzou added 2 commits February 2, 2023 03:55

Multiline JSON for equations and callbacks implemented

5ae99ae

Comment out buttons in ide.cpp and remove comments in main.cpp

b283152

This was linked to issues Feb 2, 2023

Multiline JSON strings for callbacks and equations #1342

Closed

JSON for defaults and forms #504

Closed

change from rapidjson object to array for equations and callbacks

ed62311

comment out buttons used for form conversion

a49e54f

sjanzou requested a review from brtietz February 3, 2023 10:29

brtietz approved these changes Feb 3, 2023

View reviewed changes

brtietz merged commit 99aa468 into patch Feb 3, 2023

cpaulgilman added the added to release notes PR and/or issue has been added to release notes for a public release label Feb 21, 2023

Conversation

sjanzou commented Jan 30, 2023

Uh oh!

mjprilliman left a comment

Choose a reason for hiding this comment

Uh oh!

brtietz commented Jan 30, 2023

Uh oh!

brtietz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brtietz Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

brtietz Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

brtietz Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

sjanzou Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

mjprilliman commented Jan 30, 2023

Uh oh!

brtietz commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjprilliman commented Jan 30, 2023

Uh oh!

cpaulgilman commented Jan 30, 2023

Uh oh!

sjanzou commented Jan 31, 2023

Uh oh!

sjanzou commented Jan 31, 2023

Uh oh!

brtietz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjanzou commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brtietz commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjanzou commented Feb 2, 2023 via email

Uh oh!

sjanzou commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brtietz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brtietz left a comment •

edited

Loading

brtietz commented Jan 30, 2023 •

edited

Loading

brtietz left a comment •

edited

Loading

sjanzou commented Feb 2, 2023 •

edited

Loading

brtietz commented Feb 2, 2023 •

edited

Loading

sjanzou commented Feb 3, 2023 •

edited

Loading