Better zones UI #95

willjallen · 2024-04-26T11:39:06Z

Addresses Name zones and Reorder zone tabs in #92

Add names to zones(spots) with serialization/deserialization
Support zone tab reordering
Disable closing and reordering Base tab
Fix bug where closing one tab will close multiple others

willjallen · 2024-04-26T11:40:22Z

Still a WIP. There's a bug during deserialization where opening unopened tabs makes them refresh and reorder.

chrxh · 2024-04-26T16:43:34Z

These are nice additions. Take your time and let me know when it's ready.

- c string was copied into std::string with null char since constructor had a count parameter, causing truncation when concatenating with the ID in the imgui label.

…better-zones-ui

…ult names to legacy save files.

willjallen · 2024-04-30T03:52:06Z

@chrxh ready for review

chrxh

The code looks almost fine for me (see comments there). Good work!

I have tested it and a few issues should be addressed before merging:

The order of the zones are not preserved after saving and loading a sim.
When copying simulation parameters (by pressing the copy button), rearranging the zones and then pasting the parameters, the order of the zones is not restored.
Analogous to the counting of the radiation sources (see radiation source window), I would prefer to name the zones from "Zone 1" (and not from Zone 0). Counting from 0 is not so familiar to many people (except for programmers :D)

Some minor stuff (need not necessarily be addressed in this PR, but it would be nice to fix them at some point):

The automatic naming scheme can create zone names which are already in use. An example:
Given: 2 zones named "Zone 0" and "Zone 1"
Action: delete "Zone 0" and than add a new zone. The new zone will become the name "Zone 1", which is already in use.
A reset button for the zone name, as is also available for all other properties.

Do you would like to see your feature in v4.9.2 or in v4.10.0 (this update will still take a few weeks until it will be ready)?
Tomorrow I will be one week off so I can probably not answer until I return.

chrxh · 2024-04-30T17:04:10Z

source/Gui/SimulationParametersWindow.cpp

                        parameters.spots[i] = parameters.spots[i + 1];
                        origParameters.spots[i] = origParameters.spots[i + 1];
                    }
                    --parameters.numSpots;
                    --origParameters.numSpots;
                    _simController->setSimulationParameters(parameters);
                    _simController->setOriginalSimulationParameters(origParameters);
+                }else{


I suppose there was a bug before with the counting of tab. Good that you have fixed it!

chrxh · 2024-04-30T17:04:31Z

source/Gui/SimulationParametersWindow.cpp

                    ImGui::EndTabItem();
                }
-
-                //delete spot


Please restore this comment.

chrxh · 2024-04-30T17:05:02Z

source/Gui/SimulationParametersWindow.cpp

+        if (AlienImGui::BeginTreeNode(AlienImGui::TreeNodeParameters().text("General"))) {
+            if(AlienImGui::InputText(AlienImGui::InputTextParameters().name("Name").textWidth(RightColumnWidth), _spotNameStrings[tab])){
+                _spotNameStrings[tab] = _spotNameStrings[tab].substr(0, SIM_PARAM_SPOT_NAME_LENGTH - 1); // Leave space for null byte
+                strncpy(spot.name, _spotNameStrings[tab].c_str(), SIM_PARAM_SPOT_NAME_LENGTH);


Resharper tells me that strncpy is marked as deprecated because this function can be unsafe. One should use strncpy_s instead.

strncpy_s, as well as the other _s variants, have an implementation in MSVC, but not in GCC. Thus, it is not portable and will not compile on the CI build, nor many standard linux distros. See 7421385. There are alternatives to the current method, such as using a library or using memcpy with strlen. I am agnostic to any of these options.

Oh all right, I didn't know that because I use MSVC on my machine.

chrxh · 2024-04-30T17:06:11Z

source/Gui/SimulationParametersWindow.h

@@ -41,7 +41,14 @@ class _SimulationParametersWindow : public _AlienWindow
    std::optional<int> _sessionId;
    bool _focusBaseTab = false;
    std::vector<std::string> _cellFunctionStrings;
-
+
+    std::vector<std::string> _spotNameStrings;


Is this member really necessary (same for _copiedSpotNameStrings)? Couldn't it be extracted from the name member in SimulationParameterSpot? I ask because I would like to avoid redundant data because then one always has to remember to keep it up to date in the code (e.g. after rearranging zones, pasting parameters, etc.).

I will look into this. It was necessary at one point to deal with an imgui bug, but I question if this remains the case.

chrxh · 2024-04-30T17:08:46Z

source/Gui/SimulationParametersWindow.h

+    std::vector<std::string> _copiedSpotNameStrings;
+    std::vector<std::string> _copiedSpotTabIDs;
+
+    int _serialTabID;


I would prefer to initialize PODs at the point where they are declared. So here it should probably be = 0.

willjallen · 2024-04-30T20:07:19Z

Analogous to the counting of the radiation sources (see radiation source window), I would prefer to name the zones from "Zone 1" (and not from Zone 0). Counting from 0 is not so familiar to many people (except for programmers :D)

Fair :)

Some minor stuff (need not necessarily be addressed in this PR, but it would be nice to fix them at some point):

The automatic naming scheme can create zone names which are already in use. An example:
Given: 2 zones named "Zone 0" and "Zone 1"
Action: delete "Zone 0" and than add a new zone. The new zone will become the name "Zone 1", which is already in use.

A reset button for the zone name, as is also available for all other properties.

The order of the zones are not preserved after saving and loading a sim.

When copying simulation parameters (by pressing the copy button), rearranging the zones and then pasting the parameters, the order of the zones is not restored.

Good observations. I will fix these.

Do you would like to see your feature in v4.9.2 or in v4.10.0 (this update will still take a few weeks until it will be ready)? Tomorrow I will be one week off so I can probably not answer until I return.

My regular work schedule is highly variable at the moment, but I do not think it will take weeks to make these changes. Enjoy your time off!

chrxh · 2024-04-30T20:59:09Z

My regular work schedule is highly variable at the moment, but I do not think it will take weeks to make these changes. Enjoy your time off!

Thanks!
The part about the weeks referred to changes that I still want to make and had planned for v4.10. Your changes could then also be included in that version :)

willjallen added 6 commits April 24, 2024 10:25

Zones now have modifiable names with max length of 31.

d4f6764

Merge branch 'chrxh:develop' into feat/better-zones-ui

e7d9a9d

Missing import

10c1dd7

Make base impossible to close

8ec873a

Support reordering zone tabs and fix multiple tabs closing bug

507d6e9

Support zone name (de)/serialization. Truncate imgui string before copy.

1de2d08

willjallen marked this pull request as draft April 26, 2024 11:39

Merge branch 'chrxh:develop' into feat/better-zones-ui

582a445

willjallen added 6 commits April 27, 2024 01:23

Fix reorder on deserialization bug

42cca56

- c string was copied into std::string with null char since constructor had a count parameter, causing truncation when concatenating with the ID in the imgui label.

Merge remote-tracking branch 'origin/feat/better-zones-ui' into feat/…

e89cae6

…better-zones-ui

Add copy/paste and browser download support for zone names, give defa…

dd631dc

…ult names to legacy save files.

Update strcmp include for linux

77512b5

Merge branch 'chrxh:develop' into feat/better-zones-ui

e110872

gcc does not support _s strcpy variants

7421385

willjallen marked this pull request as ready for review April 29, 2024 05:55

willjallen mentioned this pull request Apr 29, 2024

QOL Features #92

Open

19 tasks

chrxh requested changes Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better zones UI #95

Better zones UI #95

willjallen commented Apr 26, 2024 •

edited

Loading

willjallen commented Apr 26, 2024

chrxh commented Apr 26, 2024

willjallen commented Apr 30, 2024

chrxh left a comment

chrxh Apr 30, 2024

chrxh Apr 30, 2024

chrxh Apr 30, 2024

willjallen Apr 30, 2024

chrxh Apr 30, 2024

chrxh Apr 30, 2024

willjallen Apr 30, 2024

chrxh Apr 30, 2024

willjallen commented Apr 30, 2024 •

edited

Loading

chrxh commented Apr 30, 2024

Better zones UI #95

Are you sure you want to change the base?

Better zones UI #95

Conversation

willjallen commented Apr 26, 2024 • edited Loading

willjallen commented Apr 26, 2024

chrxh commented Apr 26, 2024

willjallen commented Apr 30, 2024

chrxh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willjallen commented Apr 30, 2024 • edited Loading

chrxh commented Apr 30, 2024

willjallen commented Apr 26, 2024 •

edited

Loading

willjallen commented Apr 30, 2024 •

edited

Loading