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 fullscreen option. #1017

Merged

Conversation

de-vri-es
Copy link
Contributor

This PR adds an option to enter fullscreen mode, hiding all interface elements and showing nothing but the rendered view. It makes rviz much nicer to use for showing information in demo setups.

I added the option under the view_menu_ in code, but it turns out it's actually called "Panels" in the GUI. That might make it the wrong place for this option. Alternatively the menu could be renamed to view, or a new view menu could be added.

F11 also works, and is the only way other than restarting rviz to leave fullscreen mode. Might make sense to also use Escape for that?

One point of note: I'm using setHideButtonVisibility(bool) which is a public member of VisualizationFrame. I couldn't find anything actually calling the member, but if it is exposed to plugins they could potentially interact badly with the fullscreen option. It seems unlikely though, so maybe it's better to make that member private?

@de-vri-es
Copy link
Contributor Author

Oh, I tested on a system runing xmonad without window decorations, so I'm not 100% that showFullScreen() removes the window decoration. I kind of assumed it does.

@de-vri-es de-vri-es force-pushed the kinetic-fullscreen branch 2 times, most recently from c0fc365 to 77663df Compare June 3, 2016 20:40
@de-vri-es
Copy link
Contributor Author

Updated to use signals/slot for notifying PanelDockWidgets of full screen changes to decouple them from the VisualizationFrame again.

@VictorLamoine
Copy link
Contributor

VictorLamoine commented Aug 31, 2016

I tested this PR and it works perfectly. Escape allows to exit the full screen mode.

Minor issue: the maximized window state is not retrieved when exiting full screen (the window is not maximized anymore).

Really useful for demos/screenshots.

👍

@de-vri-es
Copy link
Contributor Author

I'll see if there is a sensible way to retain maximized state. Thanks for testing!

@de-vri-es
Copy link
Contributor Author

By the way, escape does work to exit full screen mode.

@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

Did you ever find a way to preserve the maximized state?

@wjwwood wjwwood added this to the untargeted milestone Oct 17, 2016
@@ -350,6 +358,9 @@ protected Q_SLOTS:
ros::WallTime last_fps_calc_time_;

QString error_message_; ///< Error message (if any) from most recent saveDisplayConfig() call.

/// Indicates if the toolbar should be visible outside of fullscreen mode.
bool toolbar_visibile_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo, visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks btw, fixed.

@VictorLamoine
Copy link
Contributor

VictorLamoine commented Oct 17, 2016

1 - Retrieve if the window was maximized before going full screen
2 - When returning from full screen, call showMaximized instread of showNormal if the window was maximized before.

The problem is that the maximized property does not work with X11. I tested and it is always zero, it is stated in Qt docs (see above links).

void VisualizationFrame::setFullScreen( bool full_screen )
{
  Q_EMIT( fullScreenChange( full_screen ) );
  static bool was_maximized( isMaximized() );

  if (full_screen)
    toolbar_visibile_ = toolbar_->isVisible();
  else
    was_maximized = isMaximized();

  std::cout << was_maximized << std::endl;

  menuBar()->setVisible(!full_screen);
  toolbar_->setVisible(!full_screen && toolbar_visibile_);
  statusBar()->setVisible(!full_screen);
  setHideButtonVisibility(!full_screen);

  if (full_screen)
    showFullScreen();
  else if (was_maximized)
    showMaximized();
  else
    showNormal();
}

@de-vri-es
Copy link
Contributor Author

There's also QtWidget::saveGeometry() and QtWidget::restoreGeometry(). Not sure how reliable that will be, but it's worth a try. Note that I'm currently on holiday, so earliest I can try it is somewhere next week.

@VictorLamoine
Copy link
Contributor

Fixes #743

@de-vri-es
Copy link
Contributor Author

Digging through the Qt documentation once more, I found this method of setting/disabling fullscreen:

if (full_screen)
  setWindowState(windowState() | Qt::WindowFullScreen);
else
  setWindowState(windowState() & ~Qt::WindowFullScreen);
show();

This preserves all other flags such as minimized (not applicable here I think) and maximized state. I tested with a xfwm4 as window manager, and it worked as expected. Window state is preserved between entering and leaving fullscreen.

Updated and rebased the PR.

But note that I don't think it fixes #743. All widgets except for the 3D display are hidden in full-screen mode. That probably includes camera displays.

@wjwwood
Copy link
Member

wjwwood commented Jan 10, 2017

Cool, I'll have another pass at it asap.

@de-vri-es
Copy link
Contributor Author

No rush, I haven't been rushing either, but have you had the time already?

@wjwwood
Copy link
Member

wjwwood commented Feb 27, 2017

I'll come around to it before the Lunar release of rviz.

@de-vri-es
Copy link
Contributor Author

Heh, shooting for the moon. Anyway, thanks :)

@VictorLamoine
Copy link
Contributor

I tested again and it still works great.

The issue with the full-screen mode not being kept is still here but does not happen the first time. I think this really is minor and should not prevent the merging of this PR.

Here is a nice video:
https://drive.google.com/open?id=0B4_0QfSy1tFqa29fMHZCTWwxbHc

@de-vri-es
Copy link
Contributor Author

The issue with the full-screen mode not being kept is still here but does not happen the first time. I think this really is minor and should not prevent the merging of this PR.

That is weird. I tested with xfwm and the previous version did mess up maximized state after leaving full screen, but the current did not.

Though I agree its something that shouldn't be a big problem for merging. Since it's a problem with Qt / X11 I actually think it's better to ignore it. The proper place to fix it is there. Any workaround we introduce may end up breaking if Qt fixes the issue on their end, and we end up with legacy code which nobody really remembers the purpose of.

@de-vri-es de-vri-es force-pushed the kinetic-fullscreen branch from 4cf71ad to 6805749 Compare April 7, 2017 08:50
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

changes lgtm

@wjwwood
Copy link
Member

wjwwood commented May 1, 2017

Finally got around to testing this. Looks good, thanks!

@wjwwood wjwwood merged commit fae736d into ros-visualization:kinetic-devel May 1, 2017
@wxmerkt
Copy link
Contributor

wxmerkt commented May 1, 2017

Awesome, thanks! Will this be backported to indigo and released into the debians?

@wjwwood
Copy link
Member

wjwwood commented May 1, 2017

@wxmerkt I was just going to put it into Kinetic and Lunar, but since you asked nicely (and it seems low risk) I'll back port it to Indigo too.

@de-vri-es de-vri-es deleted the kinetic-fullscreen branch May 1, 2017 21:18
@de-vri-es de-vri-es restored the kinetic-fullscreen branch May 1, 2017 21:23
@de-vri-es
Copy link
Contributor Author

hmm, maybe I shouldn't have removed the branch? Yay, github has a restore branch button.

Thanks for merging.

@wjwwood
Copy link
Member

wjwwood commented May 2, 2017

@de-vri-es that's ok, I'll manually handle cherry-picking for forward/back porting.

@wxmerkt
Copy link
Contributor

wxmerkt commented May 2, 2017

Thank you @wjwwood :)

wxmerkt pushed a commit to wxmerkt/rviz that referenced this pull request May 25, 2017
wjwwood pushed a commit that referenced this pull request Jun 5, 2017
@wjwwood
Copy link
Member

wjwwood commented Jun 5, 2017

Backported in #1110

wjwwood added a commit that referenced this pull request Jun 5, 2017
* Add fullscreen option. (#1017)

* urdfdom compatibility (#1064)

* Use urdf::*ShredPtr instead of boost::shared_ptr (#1044)

urdfdom_headers uses C++ std::shared_ptr. As it exports it as custom
*SharedPtr type, we can use the to sty compatible.

This also adds a proper dependency for urdfdom-headers

* adaptions to build against both urdfdom 0.3 and 0.4

... relying on the compatibility layer of urdf package

* Update display if empty pointcloud2 is published (#1073)

Do not show last point cloud any more, if published point cloud message does not contain any points

* Correctly scale the render panel on high resolution displays (#1078)

* support multiple material for one link (#1079)

* Fixed duplicate property name for Path colors (#1089)

See issue #1087.

* fix type error in newer versions of urdf (#1098)

* Use unique material names for robot links. (#1102)

* avoid C++11 feature for back port to indigo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants