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

Allow Dialogs to have a MainWindow independent config #418

Merged
merged 46 commits into from
Jul 13, 2022

Conversation

mayman99
Copy link
Contributor

@mayman99 mayman99 commented Jun 14, 2022

Signed-off-by: Mohamad mohamad@openrobotics.org
🎉 New feature

This PR allows Dialogs to read and write to the config file.
The motivation for this PR is, all the current reading/writing of the Config is implemented in MainWindow, and the Application has not been initialized yet then there is not MainWindow object.

a dialog config looks like <dialog name=\"dialog_name\" attribute_1=\"true\"/>"

Workflow:

  1. Set dialog name SetName
  2. set the dialog default config using SetDefaultConfig
  3. Read an attribute of the dialog config using ReadAttribute
  4. if the dialog config doesn't exist in the existing config file
    5. append the dialog default config to the config file
    6. read from the written config
  5. if the dialog config exists in the existing config file, read it and return the value as string
  6. update an attribute value in the dialog config using WriteAttributre

The changes here are used in this PR.

Summary

Test it

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.

chapulina and others added 3 commits May 27, 2022 12:37
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jun 14, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Jun 14, 2022
@mayman99 mayman99 mentioned this pull request Jun 14, 2022
9 tasks
@chapulina chapulina moved this from Inbox to In progress in Core development Jun 14, 2022
@chapulina chapulina added the OOBE 📦✨ Out-of-box experience label Jun 14, 2022
@chapulina chapulina changed the base branch from ign-gui6 to chapulina/3/start_dialog June 15, 2022 19:53
@chapulina chapulina added 🏰 citadel Ignition Citadel and removed 🏯 fortress Ignition Fortress labels Jun 15, 2022
Mohamad and others added 6 commits June 16, 2022 18:25
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@mayman99 mayman99 changed the base branch from chapulina/3/start_dialog to ign-gui3 June 22, 2022 07:02
Mohamad added 5 commits June 22, 2022 09:45
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #418 (94e5bbc) into ign-gui3 (bc53940) will increase coverage by 0.29%.
The diff coverage is 76.71%.

@@             Coverage Diff              @@
##           ign-gui3     #418      +/-   ##
============================================
+ Coverage     66.12%   66.42%   +0.29%     
============================================
  Files            29       29              
  Lines          3218     3291      +73     
============================================
+ Hits           2128     2186      +58     
- Misses         1090     1105      +15     
Impacted Files Coverage Δ
include/ignition/gui/MainWindow.hh 100.00% <ø> (ø)
src/Dialog.cc 77.17% <76.71%> (+8.75%) ⬆️

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 bc53940...94e5bbc. Read the comment docs.

Mohamad added 3 commits June 23, 2022 16:36
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@mayman99
Copy link
Contributor Author

I had to comment the test here , until someone could give me some feedback, as I couldn't figure out why it was failing

@mayman99 mayman99 marked this pull request as ready for review June 23, 2022 16:32
@mayman99 mayman99 requested a review from chapulina as a code owner June 23, 2022 16:32
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@mayman99 mayman99 changed the title parse show quick setup menu by default option Allow Dialogs to have a MainWindow independent config Jun 28, 2022
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

The motivation for this PR is, all the current reading/writing of the Config is implemented in MainWindow, and the Application has not been initialized yet then there is not MainWindow object.

I'm not sure I understand this part. The application has to be initialized before the dialog. Can't we use leave the config path handling to the Application? It would be nice if we didn't need another config file and could reuse the same one. The <dialog> elements could be siblings of the <window> element.


Have you seen the WindowConfig struct? That's used to hold an in-memory representation of an XML config file, load from a file and write to it. It would be interesting to follow the same pattern for consistency of the API, creating a new DialogConfig struct.

src/Dialog.cc Outdated Show resolved Hide resolved
include/ignition/gui/Dialog.hh Outdated Show resolved Hide resolved
chapulina and others added 7 commits June 29, 2022 22:25
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@mayman99
Copy link
Contributor Author

mayman99 commented Jul 6, 2022

Updated the PR per VC, except of using templates with ReadAttributeValue, as it results in a seg-fault when using in gazebo, which needs further debugging to figure out why.

include/ignition/gui/Dialog.hh Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
include/ignition/gui/Dialog.hh Outdated Show resolved Hide resolved
src/Dialog_TEST.cc Outdated Show resolved Hide resolved
src/Dialog_TEST.cc Outdated Show resolved Hide resolved
src/Dialog_TEST.cc Outdated Show resolved Hide resolved
Mohamad added 2 commits July 6, 2022 20:28
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Mohamad added 3 commits July 8, 2022 16:17
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
include/ignition/gui/Dialog.hh Outdated Show resolved Hide resolved
src/Dialog.cc Outdated Show resolved Hide resolved
src/Dialog_TEST.cc Outdated Show resolved Hide resolved
Mohamad added 3 commits July 9, 2022 00:36
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
Signed-off-by: Mohamad <mohamad@openrobotics.org>
@mayman99
Copy link
Contributor Author

@chapulina green 👍
I didn't have time to convert the Read/Update functions to use template functions (I did but it results in a seg fault when used in gz-sim, my maintenance time was not enough to find the problem)

Core development automation moved this from In progress to In review Jul 13, 2022
@chapulina chapulina merged commit f1ff486 into ign-gui3 Jul 13, 2022
@chapulina chapulina deleted the quick_setup_menu_config branch July 13, 2022 16:19
Core development automation moved this from In review to Done Jul 13, 2022
@mayman99 mayman99 mentioned this pull request Jul 27, 2022
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel OOBE 📦✨ Out-of-box experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants