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

"Value Display" update #11895

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

gillamkid
Copy link
Contributor

Description

This PR has 2 commits

  1. Fix button sizes in TelemetryValuesBar
    • In QGC 4.4.2 these buttons look fine, but in the master branch they were super squashed looking
  2. Organize "Value Display" so it is more scannable
    • Now it is organized with keys on the left and values/inputs on the right, making the form more intuitive and scannable for first time users.
    • The design of the form is now more similar to other parts of QGC, namely the views seen in "Application Settings"

Sponsor

This contribution was sponsored by Firestorm
654d4f9476ff2a38f37e9ab9_firestorm-homepage-share-img-2

Before

before-value-display.mp4

After

after-value-display.mp4

in QGC 4.4.2 these buttons look fine, but in the master branch they were super squashed looking

Contribution Sponsor: Firestorm (launchfirestorm.com)
Now it is organized with keys on the left and values/inputs on the right, making the form more intuitive and scannable for first time users.

The design of the form is now more similar to other parts of QGC, namely the views seen in "Application Settings"

Contribution Sponsor: Firestorm (launchfirestorm.com)
@gillamkid gillamkid changed the title Gillamkid/value display update "Value Display" update Sep 18, 2024
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-sep-18-2024/40762/4

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-sep-18-2024/40762/1

@mrpollo
Copy link
Member

mrpollo commented Sep 18, 2024

Changes look good to me, its a really nice improvement.

@DonLakeFlyer Can you do one last final check here? just want to make sure nothing is lost in the transition

@DonLakeFlyer
Copy link
Contributor

@gillamkid The mp4 seem to be gone now? Maybe just images showing before after changes?

@DonLakeFlyer
Copy link
Contributor

I had taken an initial look at this a bit ago. My concern was with the sizing of the buttons for add/remove columns/row. If they are too small and right next to each other they can be tough to touch with a fat finger on a tablet. I'll need to build it myself and look.

@DonLakeFlyer
Copy link
Contributor

The mp4 seem to be gone now? Maybe just images showing before after changes?

Don't worry about this. I'm just building it myself to look at it.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 18, 2024

Hmm, this is different than I last saw it. I thought there were little buttons with images for add/remove row/columns.

Anyway:

Here is current master. You can see how the buttons are really small. This is because it uses minTouchPixels / 2 which in reality is incorrect usaget. Since it is smaller than what is touchable on a tablet. It also expose a bug in QGCButton which is not positioning the text correctly.
Screenshot 2024-09-18 at 11 16 34 AM

Here is what the editable panel looks like with this pull. My opinion is that those buttons end up being way too big and look really strange in this context:
Screenshot 2024-09-18 at 11 11 58 AM

Here is the same thing but using full minTouchPixels size which is the calculated minimum size of something which is touchable on a tablet. It also shows the text positioning bug in QGCButton which needs fixing. I think this looks way better and is still functional on a tablet from a sizing/touchable perspective.
Screenshot 2024-09-18 at 11 10 56 AM

@DonLakeFlyer
Copy link
Contributor

  • The design of the form is now more similar to other parts of QGC, namely the views seen in "Application Settings"

Yes, much better to head this direction.

Detailed comments...

Current master which you can see is kind of a mess of complexity and alignment problems:
Screenshot 2024-09-18 at 11 28 18 AM

Here is what this pull looks like. It's better from an alignment perspective. But for me all those divider lines make it look really noisy. Also the Range section needs conversion.
Screenshot 2024-09-18 at 11 33 12 AM

I think you need to take the group layout grouping further than a single group. This is how the new settings stuff works and it ends up looking much better. I also don't think there needs to be toggle visibility. And it groups things more logically. Here is a quick hack on top of your pull to show it heading more in the direction of multiple groupings. Certainly needs more work but I hope you get the idea:
Screenshot 2024-09-18 at 11 46 57 AM

@@ -42,7 +42,7 @@ RadioButton {
color: control.textColor
opacity: enabled ? 1.0 : 0.3
verticalAlignment: Text.AlignVCenter
leftPadding: control.indicator.width + (_noText ? 0 : ScreenTools.defaultFontPixelWidth * 0.25)
leftPadding: control.indicator.width + (_noText ? 0 : control.leftPadding)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? This is incorrect in that it doesn't adjust sizing based on the configurable font size.

@@ -18,6 +18,7 @@ ColumnLayout {
property string heading
property string headingDescription
property bool showDividers: true
property bool showBorder: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave this as is for now. Don't want to create a new UI paradigm of these group layout without borders until it is really needed. That is already mostly already covered by the SectionHeader control.

@DonLakeFlyer
Copy link
Contributor

DonLakeFlyer commented Sep 19, 2024

If you want I can merge this in and then fix up the rest of the stuff. All simple, take a few minutes for me. Thanks so much for cleaning this messy stuff up.

@gillamkid
Copy link
Contributor Author

If you want I can merge this in and then fix up the rest of the stuff. All simple, take a few minutes for me. Thanks so much for cleaning this messy stuff up.

Yeah that'd be great! Not sure when I'd get to it

@DonLakeFlyer DonLakeFlyer merged commit 1ad5787 into mavlink:master Sep 20, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants