Common widget GzColor (Citadel) #410

AzulRadio · 2022-06-02T20:27:10Z

New feature

Part of Provide reusable QML components with common widgets #310

Summary

Pack RGB color GUI into a common widget GzColorRGB.qml

Replace the color GUI in Grid3d with the common widget

Test it

(New)

(Deprecated)

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: youhy <[email protected]>

codecov · 2022-06-02T20:36:17Z

Codecov Report

Merging #410 (96f0078) into ign-gui3 (9b27bec) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           ign-gui3     #410   +/-   ##
=========================================
  Coverage     66.10%   66.10%           
=========================================
  Files            29       29           
  Lines          3216     3216           
=========================================
  Hits           2126     2126           
  Misses         1090     1090

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b27bec...96f0078. Read the comment docs.

Signed-off-by: youhy <[email protected]>

chapulina

Looking good! Just a few comments

include/ignition/gui/resources.qrc

include/ignition/gui/qml/GzColorRGB.qml

Signed-off-by: youhy <[email protected]>

jennuine · 2022-06-13T13:25:20Z

include/ignition/gui/qml/GzColor.qml

+
+
+GridLayout {
+ id: root


Let's name this to something else like gzColorRoot for safety

Fix in 8835c72

jennuine · 2022-06-13T13:32:46Z

include/ignition/gui/qml/GzColor.qml

+ property alias b: b_spin.value
+ property alias a: a_spin.value
+
+ property bool textVisible: false


nit: textVisible isn't very descriptive. Can we rename this to showColorText?

Remove textVisible in 8835c72

Signed-off-by: youhy <[email protected]>

jennuine

Looks good so far! A couple of things,

Anytime the color dialog is open, this warning appears in the terminal:

Gtk-Message: 11:24:17.963: GtkDialog mapped without a transient parent. This is discouraged.

This happened before anytime the native dialog is used (but doesn't happen with Fortress' material color dialog). It would be nice to resolve it so it doesn't spam the console if a user changes colors often.

Also, when I select a custom color, the color appears twice next to the custom color section:

include/ignition/gui/qml/GzColor.qml

src/plugins/grid_3d/Grid3D.qml

Signed-off-by: youhy <[email protected]>

AzulRadio · 2022-06-17T18:48:01Z

@jennuine

Anytime the color dialog is open, this warning appears in the terminal:
Gtk-Message: 11:24:17.963: GtkDialog mapped without a transient parent. This is discouraged.

I don't have this warning on Ogre but I think I might know how to fix this. An attempt to fix is included in 86d1b1a.

Also, when I select a custom color, the color appears twice next to the custom color section

This seems to be a bug of Qt. Even if I use the example code from their official page, this bug still persists. https://doc.qt.io/qt-5/qml-qtquick-dialogs-colordialog.html

All the other changes have also been included to commit 86d1b1a

jennuine

Thanks for iterating. A few more minor comments.

I don't have this warning on Ogre but I think I might know how to fix this. An attempt to fix is included in 86d1b1a.

I didn't see the attempt to fix the warning. Was it included in that commit? The warning still appears.

Also, when I select a custom color, the color appears twice next to the custom color section

This seems to be a bug of Qt. Even if I use the example code from their official page, this bug still persists. https://doc.qt.io/qt-5/qml-qtquick-dialogs-colordialog.html

I see, it does the same w/o this PR too. Strange, this wasn't an issue before. Anyways, thanks for looking into this

include/ignition/gui/qml/GzColor.qml

Signed-off-by: youhy <[email protected]>

jennuine

LGTM, nice work!

Anytime the color dialog is open, this warning appears in the terminal:
Gtk-Message: 11:24:17.963: GtkDialog mapped without a transient parent. This is discouraged.

This message is coming from the OS's native dialog, not from this PR. The fix would be to use Qt's color dialog but there is no way to easily use that through QML currently.

All the requested changes have been made.

AzulRadio added 2 commits June 2, 2022 09:56

common widget GzColorRGB

ae33b8a

Signed-off-by: youhy <[email protected]>

change Grid3D to common widget

1ffcc4c

Signed-off-by: youhy <[email protected]>

github-actions bot added the 🏰 citadel Ignition Citadel label Jun 2, 2022

AzulRadio added 2 commits June 3, 2022 08:09

fix white grid bug

ebdfaf0

Signed-off-by: youhy <[email protected]>

change alpha to 0 - 1

814da4c

Signed-off-by: youhy <[email protected]>

chapulina added the OOBE 📦✨ Out-of-box experience label Jun 3, 2022

chapulina previously requested changes Jun 3, 2022

View reviewed changes

code clean up

f6fe822

Signed-off-by: youhy <[email protected]>

AzulRadio marked this pull request as ready for review June 3, 2022 20:30

AzulRadio requested a review from jennuine as a code owner June 3, 2022 20:30

AzulRadio changed the title ~~Add common widget for color~~ Common widget GzColor (Citadel) Jun 3, 2022

AzulRadio added 2 commits June 6, 2022 08:19

ignSpinbox fillwidth

622a607

Signed-off-by: youhy <[email protected]>

change colorname to the other row

b757ec8

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 13, 2022

View reviewed changes

AzulRadio added 3 commits June 13, 2022 14:28

rewrite GzColor, only color dialogue

8835c72

Signed-off-by: youhy <[email protected]>

change Grid3D to color dialogue only

1af7bd5

Signed-off-by: youhy <[email protected]>

change Rectangle to Item

0388151

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 16, 2022

View reviewed changes

change rgba range to 0-1.0 and other minor fixes

86d1b1a

Signed-off-by: youhy <[email protected]>

jennuine reviewed Jun 20, 2022

View reviewed changes

include/ignition/gui/qml/GzColor.qml Outdated Show resolved Hide resolved

include/ignition/gui/qml/GzColor.qml Outdated Show resolved Hide resolved

jennuine reviewed Jun 20, 2022

View reviewed changes

include/ignition/gui/qml/GzColor.qml Outdated Show resolved Hide resolved

AzulRadio and others added 2 commits June 22, 2022 13:58

change a few component id

56dbef5

Signed-off-by: youhy <[email protected]>

Merge branch 'ign-gui3' into azulradio/common_widget

96f0078

jennuine approved these changes Jun 28, 2022

View reviewed changes

AzulRadio merged commit f29fe15 into ign-gui3 Jun 28, 2022

AzulRadio deleted the azulradio/common_widget branch June 28, 2022 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common widget GzColor (Citadel) #410

Common widget GzColor (Citadel) #410

AzulRadio commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading

chapulina left a comment

jennuine Jun 13, 2022

AzulRadio Jun 13, 2022 •

edited

Loading

jennuine Jun 13, 2022

AzulRadio Jun 13, 2022

jennuine left a comment

AzulRadio commented Jun 17, 2022 •

edited

Loading

jennuine left a comment

jennuine left a comment •

edited

Loading

Common widget GzColor (Citadel) #410

Common widget GzColor (Citadel) #410

Conversation

AzulRadio commented Jun 2, 2022 • edited Loading

New feature

Summary

Test it

Checklist

codecov bot commented Jun 2, 2022 • edited Loading

Codecov Report

chapulina left a comment

Choose a reason for hiding this comment

jennuine Jun 13, 2022

Choose a reason for hiding this comment

AzulRadio Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

jennuine Jun 13, 2022

Choose a reason for hiding this comment

AzulRadio Jun 13, 2022

Choose a reason for hiding this comment

jennuine left a comment

Choose a reason for hiding this comment

AzulRadio commented Jun 17, 2022 • edited Loading

jennuine left a comment

Choose a reason for hiding this comment

jennuine left a comment • edited Loading

Choose a reason for hiding this comment

AzulRadio commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 2, 2022 •

edited

Loading

AzulRadio Jun 13, 2022 •

edited

Loading

AzulRadio commented Jun 17, 2022 •

edited

Loading

jennuine left a comment •

edited

Loading