Add support for resizing by dragging the title bar
ClosedPublic

Authored by ryanmccoskrie on Jul 3 2019, 2:24 AM.

Details

Summary

This is for the benefit of users who find changing the window height
through the configuration window overly tedious.

This may cause slight flickering during resizing.

FEATURE: 154686
FIXED-IN: 20.04.0

Test Plan

Run patched Yakuake.
Drag Titlebar up and down the screen.

Yakuake should automatically update its height configuration while being dragged.

Diff Detail

Repository
R369 Yakuake
Lint
Lint Skipped
Unit
Unit Tests Skipped
ryanmccoskrie requested review of this revision.Jul 3 2019, 2:24 AM
ryanmccoskrie created this revision.
ryanmccoskrie edited the summary of this revision. (Show Details)Jul 3 2019, 2:45 AM
ryanmccoskrie added a reviewer: Yakuake.
ryanmccoskrie edited the summary of this revision. (Show Details)
ryanmccoskrie added a project: Yakuake.

Got the whole change into request, not just a one-line touch up.

ngraham edited the summary of this revision. (Show Details)Jul 5 2019, 12:35 PM
ngraham added a reviewer: hein.
ngraham added inline comments.
app/titlebar.cpp
143

KDE coding style: space between if and ( and also between the ) and {

144

No need to use this; just do parent() directly (and in fact, this means you don't even need to declare it as a variable, just use it directly in the below lines.

149

space between if and (

150

KDE coding style: use braces even for single-line statements following conditionals

151

wrong indentation. Also, this could be an else if

152

use braces

app/titlebar.h
59

Indentation doesn't match the above lines

Adjusted formatting to fit KDE style guidelines.

Kept the MainWindow* parent declaration as it is needed for compilation.
Did not change to if/else as no subsequent else statement follows.

ryanmccoskrie marked 6 inline comments as done.Jul 7 2019, 3:55 AM
ryanmccoskrie added inline comments.
app/titlebar.cpp
144

This is needed to access getDesktopGeomotry. Otherwise the compiler returns that QObject has no such function.

151

As there is no following else state so that would just look weird.

ryanmccoskrie marked 2 inline comments as done.Jul 7 2019, 3:57 AM
hein added a comment.Jul 14 2019, 8:21 AM

The resize should happen in the same steps as the config allows (and reads/writes), otherwise the UI will end up being confused, in particular the menu.

app/titlebar.cpp
144

Coding style: Please don't use a C-style cast, use a C++ one.

ryanmccoskrie updated this revision to Diff 63768.EditedAug 14 2019, 10:21 PM
ryanmccoskrie edited the test plan for this revision. (Show Details)
ryanmccoskrie set the repository for this revision to R369 Yakuake.

Switched from C style cast to dynamic cast.

ryanmccoskrie added a comment.EditedAug 14 2019, 10:24 PM
In D22227#495177, @hein wrote:

The resize should happen in the same steps as the config allows (and reads/writes), otherwise the UI will end up being confused, in particular the menu.

I re-read the MainWindow::setWindowHeight code, it updates the configuration so UI confusion will not occur.

andrewfhou requested changes to this revision.Feb 29 2020, 5:58 PM
andrewfhou added a subscriber: andrewfhou.

Resizing the window works great, and the 'flickering' effect is relatively minor. However, the cursor should change when it is over the titlebar to indicate to the user that they are able to drag to resize it - ideally the vertical double arrow cursor icon

This revision now requires changes to proceed.Feb 29 2020, 5:58 PM

@ryanmccoskrie sorry this got lost. Would you be interested in finishing it up so we can get it landed?

@ryanmccoskrie sorry this got lost. Would you be interested in finishing it up so we can get it landed?

Gladly. Getting onto it right now

The Title Bar now indicates it is resizeable with a vertical size cursor.

Fixed menu buttons using vertical-size cursor.

Dragging and icon seem to be working for me! I think it should be good once you take care of the style change requests

  • Merge branch 'master' into 154686-Resize_on_title_drag
  • * Now conforms to KDE style guidelines
  • * C++ dynamic cast instead of C cast
  • * renamed parent to window in TitleBar::mouseMoveEvent()
  • Merge branch 'master' into 154686-Resize_on_title_drag
  • Merge branch 'master' into 154686-Resize_on_title_drag
  • TitleBar now uses the vertical-resize cursor
  • Fixed titlebar buttons using vertical-size cursor

Fixed formatting and added comments

ngraham accepted this revision.Mar 8 2020, 4:07 PM

LGTM. @mweepigeon? Since you requested changed, you'll nedd to re-review and change your status to Accepted if you're happy with it now.

andrewfhou accepted this revision.Mar 8 2020, 4:10 PM

Awesome!

This revision is now accepted and ready to land.Mar 8 2020, 4:10 PM

@ryanmccoskrie can you provide an email address so we can land this patch with correct attribution? Thanks again for the lovely contribution!

Added copyright statement

On a related note, I uploaded another patch that reformats the rest of Yakuake's if statements to match the ones here.

ngraham added inline comments.Mar 10 2020, 2:04 AM
app/titlebar.cpp
3

You've been working on Yakuake since 2008? :)

Fixed copyright statements.

ngraham edited the summary of this revision. (Show Details)Mar 10 2020, 4:08 PM
This revision was automatically updated to reflect the committed changes.
muesli added a subscriber: muesli.Mar 10 2020, 5:44 PM
muesli added inline comments.
app/titlebar.cpp
157

Those values (100 and 10) seem a bit arbitrary to me. Is there any reasoning behind them?

muesli added inline comments.Mar 10 2020, 5:46 PM
app/titlebar.cpp
157

Sorry, ignore my comment. Didn't realize newHeight is a percentage here. Could potentially rename the variable to make that clearer.