-
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
Improve fullscreen mode [DISCL-384] #90
Conversation
ping, there is another PR waiting after this one :-) |
@@ -3,6 +3,15 @@ Changelog {#changelog} | |||
|
|||
# Release 1.2 (git master) | |||
|
|||
* [90](https://github.com/BlueBrain/Tide/pull/90): | |||
The fullscreen mode has some new features: | |||
- Users can now enlarge and pan contents (without the need to use the zoom) |
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.
does sound more confusing than actually solving a problem. Maybe I need to see it first or a another description which is more user-oriented :)
* [90](https://github.com/BlueBrain/Tide/pull/90): | ||
The fullscreen mode has some new features: | ||
- Users can now enlarge and pan contents (without the need to use the zoom) | ||
- A double-tap toggles between *adjusted* and *maximized* fullscreen |
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.
again confused. maximized fullscreen sounds wrong: why is fullscreen not maximized by default? If it's meant to replace the focus mode, did you remove it 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, so my descriptions are not clear :-( Maybe we can discuss how to improve that.
There are two fullscreen presets, like a movie player on a phone: one fullscreen without cropping but with black borders, and one maximized fullscreen with no black borders but then the content is cropped. Double tap alternates between the two presets, but users can also pinch and pan to adjust differently or zoom in even more (useful for big images). This really only improves the fullscreen mode and implies no changes to the focus mode.
_apply( fullscreenCoordinates ); | ||
} break; | ||
|
||
case SIZE_FULLSCREEN_MAX: |
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.
why not having only this one? I probably miss something why two fullscreen behaviors are needed
@@ -152,7 +153,10 @@ QRectF LayoutEngine::_getNominalCoord( const ContentWindow& window ) const | |||
|
|||
QSizeF size = window.getCoordinates().size(); | |||
size.scale( maxSize, Qt::KeepAspectRatio ); | |||
window.getController()->constrainSize( size ); | |||
ContentWindowController( const_cast<ContentWindow&>( window ), |
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.
the window is not supposed to be modified from here. Providing another constructor seems better.
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 agree the const_cast is ugly, but I couldn't think of a simple way around it - the ContentWindow& is a class member of the Controller, so there can't be another constructor with a const ContentWindow&. Maybe by templating the class, or creating a separate one, but that looked seems like an overkill for this single occurence.
The ContentWindowController has also been separated from the ContentWindow. It had no reason to be serialized and exposed on wall applications. This was a legacy from the initial implementation of the focus mode before coordinates were stored in the class.
Updated with bugfix for the size constraints (the culprit was ContentWindowController::constrainSize when minSize > maxSize) |
+1 |
No description provided.