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

feat: Configurable tool button style kinetic #1307

Merged

Conversation

reinzor
Copy link
Contributor

@reinzor reinzor commented Oct 25, 2018

When using a lot of tools, it's annoying that not all tools fit in the default toolbar. This PR adds an additional tool menu (next to the remove tool menu) for configuring the tool button style: http://doc.qt.io/archives/qt-4.8/qt.html#ToolButtonStyle-enum

@reinzor reinzor changed the title feat: Configurable tool button style feat: Configurable tool button style kinetic Oct 25, 2018
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.

Thanks for the PR, it looks generally good to me. Some details I noticed:

  • The ➕ button is being affected by the settings, but not the ➖. It would be nice if they worked consistently.
  • On text-only mode, the ➕ button disappears, but you can find it by hovering. It would be nice if it didn't disappear. It also looks weird with text on the side and below, because there's some whitespace for the empty text. Maybe the ➕ button should always be icon-only (i.e. it doesn't respond to style changes, like ➖ currently does).
  • The toolbar style tool button itself can't be removed using the remove menu, it would be nice to give users this choice.

toolbuttonrviz

src/rviz/visualization_frame.h Show resolved Hide resolved
Also introduced separator between the toolbar actions and the static
buttons. Clarifies the separation and this way we can keep the button
ordening.
@reinzor
Copy link
Contributor Author

reinzor commented Oct 26, 2018

Thanks for the review, I updated the PR in order to address your first two points. I don't know how to address the last point.

Now I clearly made a separation between the "actions" (added dynamically via the toolbar_manager) and the static tool buttons. If we want the user to also control the visibility of the static buttons, an additional menu should be added but I think this is beyond the scope of this PR.

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.

Thank you for addressing the comments. The separator is a nice touch! And I think that the clear separation means it's ok that the buttons to the right of the separator can't be added or removed.

toolbuttonrviz2

I just have one final comment about ABI. I think it may be worth it running this branch through an ABI checker before merging, you could try it locally as explained here.

src/rviz/visualization_frame.h Outdated Show resolved Hide resolved
@reinzor
Copy link
Contributor Author

reinzor commented Oct 29, 2018

Thanks for your comment, I updated both PR's to maintain the ABI

@chapulina

This comment has been minimized.

1 similar comment
@wjwwood

This comment has been minimized.

@wjwwood
Copy link
Member

wjwwood commented Nov 2, 2018

@dirk-thomas CI doesn't seem to be getting trigger now (previous commits on this pr were tested), what's the best way to debug it?

@dirk-thomas
Copy link
Contributor

dirk-thomas commented Nov 2, 2018

You need to use the default phrase of the Jenkins plugin: see https://plugins.jenkins.io/ghprb

@ros-pull-request-builder retest this please

Update: this comment has triggered the following builds: http://build.ros.org/view/Kpr/job/Kpr__rviz__ubuntu_xenial_amd64/220/ and http://build.ros.org/view/Kpr/job/Kpr__rviz__ubuntu_xenial_amd64/221/

@wjwwood
Copy link
Member

wjwwood commented Nov 2, 2018

@dirk-thomas it looks like it is rerunning CI on the second latest commit but no the latest.

@dirk-thomas
Copy link
Contributor

it looks like it is rerunning CI on the second latest commit but no the latest.

No, the referenced checks in the GitHub UI are pointing to a build of the latest commit.

@wjwwood
Copy link
Member

wjwwood commented Nov 2, 2018

Weird, it didn't look like that before. Ok, well I dunno I guess it's working fine, thanks!

@chapulina
Copy link
Contributor

Thanks @wjwwood and @dirk-thomas !

@chapulina chapulina merged commit 9114606 into ros-visualization:kinetic-devel Nov 2, 2018
rhaschke pushed a commit that referenced this pull request Feb 17, 2019
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