-
Notifications
You must be signed in to change notification settings - Fork 16
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
Refine user interaction [DISCL-394] #95
Conversation
@@ -2,8 +2,16 @@ Changelog {#changelog} | |||
============ | |||
|
|||
# Release 1.2 (git master) | |||
|
|||
* [95](https://github.com/BlueBrain/Tide/pull/95): | |||
More consistent and intuitive user experience: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this also into the commit message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done
@@ -144,7 +144,7 @@ class ContentWindow : public Rectangle | |||
/** @return the unique identifier for this window. */ | |||
const QUuid& getID() const; | |||
|
|||
/** Is the window a panel */ | |||
/** Is the window a panel. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@return true if window is a panel
Maybe describe even shortly what a panel is/differentiates from a 'normal' window. I don't know now actually :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for the the first part.
I'm not surprised that you don't know because it has been an evolving and never clearly defined concept. It was introduced as a generalisation intended to replace the special handling applied to the "Dock" window. The WindowType enum gives the following description:
PANEL // A panel window - always interative, cannot be focused
I'm going to update it to:
PANEL // an overlay window without a control bar, cannot be focused or fullscreen
@@ -110,7 +112,7 @@ class DisplayGroup : public Rectangle, | |||
* Replace the content windows. | |||
* @param contentWindows The list of windows to set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
param name wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, done
@@ -146,6 +151,10 @@ class DisplayGroup : public Rectangle, | |||
/** Remove a window from the set of focused windows. */ | |||
void removeFocusedWindow( ContentWindowPtr window ); | |||
|
|||
|
|||
/** Get the set of focused windows. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, focus == panel? If so, unify the naming then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch! No that's a copy-paste error, thanks for noticing. Right now the only panel is the Launcher. I think there could be more in the future, like a "Help" panel that could be displayed when the wall is empty or on a tapAndHold on the background, but let's see. Even if there is only one, I find it cleaner to have this concept rather than having methods such as: "isLauncher()", "getLauncherWindow()", etc..
@@ -200,6 +212,7 @@ class DisplayGroup : public Rectangle, | |||
ar & _contentWindows; | |||
ar & _focusedWindows; | |||
ar & _fullscreenWindow; | |||
ar & _panels; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_panel and _focusWindows are not the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, see above
auto focusedWindows = ContentWindowSet{}; | ||
|
||
for( const auto& window : _group.getContentWindows( )) | ||
if( window->getControlsVisible( )) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would introduce a isSelected() state (there was one in the original version iirc). You can implement it with getControlsVisible(), but it's clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right, maybe even rename "controlsVisible" to "selected" entirely, as this is becoming just a side-effect.
@@ -151,7 +151,8 @@ void DisplayGroupRenderer::setDisplayGroup( DisplayGroupPtr displayGroup ) | |||
// of the focus context may not always be restored to its original value. | |||
// See JIRA issue: DISCL-305 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still an issue with recent Qt versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last time I checked it was still the case with Qt 5.4, if it goes away it will be by pure luck...
- Double-tap a window to make it fullscreen - To present multiple contents side by side, select them on the Desktop with a single tap then press any of their "eye" icons. - Tap the background to exit any presentation mode. - On the Desktop, tap the background to unselect all windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Should be validated by testing on the wall before merging