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 simple UI to show Overwrite-Falg in SEI-Editor #186

Merged
merged 11 commits into from
Aug 29, 2023

Conversation

franzTobiasDLR
Copy link
Member

@franzTobiasDLR franzTobiasDLR commented Aug 24, 2023

Show Override-State in SEI-Editor.

Requiers following small change in core:
virtualsatellite/VirtualSatellite4-Core@8327baa

grafik

Editor:
Untitled
For calculated properties, editor cannot be opened...

@franzTobiasDLR franzTobiasDLR added comfort/usasbility Usability improvement ui Affects the user interface labels Aug 24, 2023
@franzTobiasDLR franzTobiasDLR added this to the Release 4.16.0 milestone Aug 24, 2023
@franzTobiasDLR franzTobiasDLR self-assigned this Aug 24, 2023
@franzTobiasDLR
Copy link
Member Author

Should work after this action completed and deployed on core-integration:
virtualsatellite/VirtualSatellite4-Core@8327baa

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #186 (7e953b8) into integration (dedfa36) will not change coverage.
The diff coverage is n/a.

❗ Current head 7e953b8 differs from pull request most recent head cb36e50. Consider uploading reports for the commit cb36e50 to get more accurate results

Impacted file tree graph

@@              Coverage Diff               @@
##             integration     #186   +/-   ##
==============================================
  Coverage          87.73%   87.73%           
  Complexity           514      514           
==============================================
  Files                 74       74           
  Lines               1737     1737           
  Branches             211      211           
==============================================
  Hits                1524     1524           
  Misses               141      141           
  Partials              72       72           

@franzTobiasDLR
Copy link
Member Author

franzTobiasDLR commented Aug 24, 2023

I updated the UI so that the column is the most right column to not disturb engineers and added the hint <<calculated>> if the property is calculated. The "<<>>" should make clear that this is not an actuall value of override...

Also i moved the calculator symbol to the property value. This way it might be more clear why you cannot change this value... Although it doesn't look that hamonic anymore...

What do you think of the icons?

Do you have opinions on that? @dellerDLR @pchrszon-dlr

grafik

@pchrszon-dlr
Copy link
Contributor

I like the idea of having the calculated icons in the Value column. Sure, the alignment of the numbers is off, but I don't mind it that much. I don't know if this is possible, but it might help to have the numbers in the Value column aligned on the right, instead of on the left.

Regarding the icons in the Override column: I immediately understood the one for 'false', but I would have to guess what the 'true' one means. Is it a crossed-out link?

Maybe the Override column would be easier to understand, if the possible entries would be "inherited", "overridden", and "calculated". For "true" and "false", you always have to read the entries with respect to the column header, which is (at least for me) not as intuitive.

@franzTobiasDLR
Copy link
Member Author

Here another UI Option: This way values are alligned again, but now it might be less clear again that some numbers are calculated and cannot be edited...

Any opinons?

grafik

@pchrszon-dlr
Copy link
Contributor

I like the last option better than the previous one. It is somewhat similar to what we had before (with the icons in the Name column), but now the icons are closer to the actual values. I think in connection with the Override column, which explicitly states that some values are calculated, it is quite clear (to me) what can be edited and what can't be.

@franzTobiasDLR
Copy link
Member Author

Latest version:
-> I added the icons on names again, otherwise it looks weird if we add a mode specific value. THe vales now have the float icon:
grafik

@dellerDLR
Copy link
Contributor

I think I also prefer to have the icons on the value column.
I prefer the latest version, here it is clear to me what is overridden and what is not.

@dellerDLR
Copy link
Contributor

What about systemmodes?
You can decide on a per system mode basis if you inherit the value or override it, right?
image

@franzTobiasDLR
Copy link
Member Author

What do you think of grey font for calculated parameters?

grafik

@pchrszon-dlr
Copy link
Contributor

That looks quite nice! I think it would also be an option to use the grey foreground for inherited values instead of calculated values. But this depends on which property (inheritance/calculated) we want to emphasize.

@franzTobiasDLR
Copy link
Member Author

That looks quite nice! I think it would also be an option to use the grey foreground for inherited values instead of calculated values. But this depends on which property (inheritance/calculated) we want to emphasize.

I actually prefer calculated as the color somewhat implies that the row is 'disabled' and thus read-only. Which is not the case for inherited values.

@pchrszon-dlr
Copy link
Contributor

Yes, that's true. It would be fine with me to use the grey color for the calculated values then.

@franzTobiasDLR
Copy link
Member Author

franzTobiasDLR commented Aug 25, 2023

Last version for today: now with system mode support:
grafik

@dellerDLR
Copy link
Contributor

That looks quite nice! I think it would also be an option to use the grey foreground for inherited values instead of calculated values. But this depends on which property (inheritance/calculated) we want to emphasize.

I actually prefer calculated as the color somewhat implies that the row is 'disabled' and thus read-only. Which is not the case for inherited values.

I would agree to make the calculated rows grey since, as tobi said, you can't change them.

@franzTobiasDLR franzTobiasDLR changed the title WIP: Add simple UI to show Overwrite-Falg in SEI-Editor Add simple UI to show Overwrite-Falg in SEI-Editor Aug 28, 2023
@franzTobiasDLR
Copy link
Member Author

@dellerDLR @pchrszon-dlr I'm done with this PR now.

Did some code quality improvements and now made sure that mode parameters can only be edited if it makes sense (before you could always change them). Also make sure mode parameters are highlighted correctly (override / inherited / calculated) and for that implemented a method that evaluates if mode values are calculated or not...

Do you want to have another look at this PR?

@pchrszon-dlr
Copy link
Contributor

I checked the two recent commits and see no issues. You still have my approval.

@dellerDLR
Copy link
Contributor

dellerDLR commented Aug 29, 2023

I tested the UI and everything seems to work.
I really like the new UI!

@dellerDLR dellerDLR merged commit a98f49c into integration Aug 29, 2023
9 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comfort/usasbility Usability improvement ui Affects the user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants