Bug 356869 - kwin 5.5.1 - decoration area spacing not updated after adding/removing buttons
Summary: kwin 5.5.1 - decoration area spacing not updated after adding/removing buttons
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: kdecorations (show other bugs)
Version: 5.5.1
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: 5
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-18 14:03 UTC by Till Schäfer
Modified: 2015-12-27 16:44 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
the normal state of the decoration (68.08 KB, image/png)
2015-12-18 14:04 UTC, Till Schäfer
Details
the state of the decoration after adding a button (67.83 KB, image/png)
2015-12-18 14:04 UTC, Till Schäfer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Till Schäfer 2015-12-18 14:03:55 UTC
when adding or removing a button for the window decoration in "System Settings -> Application Style -> Window Decorations -> Buttons" and then pressing apply, the new buttons are added or removed from the decoration, but the spacing is not update. I.e. far right button is moved out of the decoration area (adding) or the right button is visible twice with only the left one working correctly (removing).  One need to close and reopen the application to get rid of this glitch. 

See the attached screen shots as an example for adding a button. 

Reproducible: Always
Comment 1 Till Schäfer 2015-12-18 14:04:27 UTC
Created attachment 96170 [details]
the normal state of the decoration
Comment 2 Till Schäfer 2015-12-18 14:04:52 UTC
Created attachment 96171 [details]
the state of the decoration after adding a button
Comment 3 Martin Flöser 2015-12-18 14:10:13 UTC
I assume resizing the window will fix it?
Comment 4 Till Schäfer 2015-12-18 14:29:03 UTC
yes, resizing fixes it.
Comment 5 Martin Flöser 2015-12-18 15:20:42 UTC
ok, so looks like just an update is missing somewhere.
Comment 6 Thomas Lübking 2015-12-18 15:37:14 UTC
Seems to be only reproducible with breeze?
How does the plastik deco behave on your side?
Comment 7 Till Schäfer 2015-12-18 15:54:12 UTC
I can confirm, that plastik is unaffected.
Comment 8 Thomas Lübking 2015-12-18 16:03:44 UTC
I actually think we had this before...
Comment 9 Hugo Pereira Da Costa 2015-12-27 11:36:46 UTC
this is not from breeze. (adding ::update all over the place here does not change anything to the issue)

most likely kdecoration2
Comment 10 Thomas Lübking 2015-12-27 12:53:34 UTC
I don't think it's a repaint, but a relayout error, ie. missing ::updateButtonsGeometry() call - you probably need to connect that to DecorationButtonGroup::geometryChanged() for m_*Buttons?
Comment 11 Hugo Pereira Da Costa 2015-12-27 13:44:12 UTC
From my recollection (but I can re-double check), updateButtonGeometry *is* called (when would it be otherwise ?). But this is not reflected on the screen.

Ok I will double check.
Comment 12 Hugo Pereira Da Costa 2015-12-27 14:22:26 UTC
(In reply to Thomas Lübking from comment #10)
> I don't think it's a repaint, but a relayout error, ie. missing
> ::updateButtonsGeometry() call - you probably need to connect that to
> DecorationButtonGroup::geometryChanged() for m_*Buttons?

So: checked
updateButtonGeometry is not called.
adding the connection "helps" but does not fix (not always): not for instance when removing buttons
also, I am confused about the suggested change: 
updateButtonsGeometry does change the DecorationButtonGroup geometry. so at least API wide, this does not look like the most intuitive connection to make ... 
maybe better Decorationbutton created/destroyed/geometry changed ? 
(that would make more sense, I guess ...)
Comment 13 Thomas Lübking 2015-12-27 14:30:41 UTC
Just looked up the buttongroup code for emitted signals (and geometryChanged was the only called)
Connecting the canged signal of the deco settings should do as well.
Comment 14 Thomas Lübking 2015-12-27 14:31:06 UTC
ps: connecting it delayed!
Comment 15 Hugo Pereira Da Costa 2015-12-27 15:20:39 UTC
Tried to connect (delayed) decorationButtonsLeftChanged and decorationButtonsRightChanged from DecorationSettings.
Works when adding buttons
not when removing.
Also, (OT) in the meanwhile, I've noticed that KDecoration2::DecoratedClient::widthChanged is called at every window move event. (without resize). 
Should I file a separate bug report ?
Comment 16 Thomas Lübking 2015-12-27 15:33:11 UTC
I guess I should write a virtuality deco ;-)

> Works when adding buttons not when removing.
That's rather odd, but there's probably even better:

void DecorationSettings::decorationButtonsLeftChanged(const QVector<KDecoration2::DecorationButtonType>&);
and
void DecorationSettings::decorationButtonsRightChanged(const QVector<KDecoration2::DecorationButtonType>&);

> Should I file a separate bug report ?
Sure, though it looks like the only emitter is
Window::setGeometry(const QRect &rect) and there is
if (rect.width() != oldRect.width()) {
        emit window()->widthChanged(rect.width());
    }

So either the old or the new geometry would alway be junk....
Comment 17 Hugo Pereira Da Costa 2015-12-27 15:42:33 UTC
(In reply to Thomas Lübking from comment #16)
> I guess I should write a virtuality deco ;-)
> 
> > Works when adding buttons not when removing.
> That's rather odd, but there's probably even better:
> 
> void DecorationSettings::decorationButtonsLeftChanged(const
> QVector<KDecoration2::DecorationButtonType>&);
> and
> void DecorationSettings::decorationButtonsRightChanged(const
> QVector<KDecoration2::DecorationButtonType>&);
> 
hmm
that's the one I used

> > Should I file a separate bug report ?
> Sure, though it looks like the only emitter is
> Window::setGeometry(const QRect &rect) and there is
> if (rect.width() != oldRect.width()) {
>         emit window()->widthChanged(rect.width());
>     }
> 
> So either the old or the new geometry would alway be junk....
Comment 18 Thomas Lübking 2015-12-27 15:55:03 UTC
"changed" is indeed a functor pointing one of them, so that's correct.

Though what's odd is that those changed events should wipe and recreate the button groups, so either the group is not updated when removing a button or you should get the signal...

-> Don't you get the signal or does ::updateButtonGeometry fail?
Comment 19 Hugo Pereira Da Costa 2015-12-27 16:21:21 UTC
(In reply to Thomas Lübking from comment #18)
> "changed" is indeed a functor pointing one of them, so that's correct.
> 
> Though what's odd is that those changed events should wipe and recreate the
> button groups, so either the group is not updated when removing a button or
> you should get the signal...
> 
> -> Don't you get the signal or does ::updateButtonGeometry fail?

Getting somewhere :)
So: signal is sent (and recieved), updateButtonGeometry succeeds. But what's missing in case of button removal, is a repaint (::update())

Combination of both seems to be working in all cases.
Comment 20 Hugo Pereira Da Costa 2015-12-27 16:27:25 UTC
Git commit ed08414b124e85b895ad3f553d590377c27af131 by Hugo Pereira Da Costa.
Committed on 27/12/2015 at 16:26.
Pushed by hpereiradacosta into branch 'master'.

Call updateButtonGeometry after decorationButtonsLeftChanged and decorationButtonsRightChanged
added ::update at the end of updateButtonGeometry

M  +12   -0    kdecoration/breezedecoration.cpp
M  +1    -0    kdecoration/breezedecoration.h

http://commits.kde.org/breeze/ed08414b124e85b895ad3f553d590377c27af131
Comment 21 Hugo Pereira Da Costa 2015-12-27 16:28:09 UTC
Git commit 768ae398483060740137331d4483aa4ecd4e005e by Hugo Pereira Da Costa.
Committed on 27/12/2015 at 16:28.
Pushed by hpereiradacosta into branch 'Plasma/5.5'.

Call updateButtonGeometry after decorationButtonsLeftChanged and decorationButtonsRightChanged
added ::update at the end of updateButtonGeometry

M  +12   -0    kdecoration/breezedecoration.cpp
M  +1    -0    kdecoration/breezedecoration.h

http://commits.kde.org/breeze/768ae398483060740137331d4483aa4ecd4e005e
Comment 22 Hugo Pereira Da Costa 2015-12-27 16:30:04 UTC
PS: thanks for the help.
Will also fix oxygen.
Comment 23 Hugo Pereira Da Costa 2015-12-27 16:35:24 UTC
Git commit 6afe7689a030c00351fb80cffacbf8c63691fee8 by Hugo Pereira Da Costa.
Committed on 27/12/2015 at 16:35.
Pushed by hpereiradacosta into branch 'Plasma/5.5'.

Call updateButtonGeometry after decorationButtonsLeftChanged and decorationButtonsRightChanged
added ::update at the end of updateButtonGeometry

M  +11   -0    kdecoration/oxygendecoration.cpp
M  +1    -0    kdecoration/oxygendecoration.h

http://commits.kde.org/oxygen/6afe7689a030c00351fb80cffacbf8c63691fee8
Comment 24 Hugo Pereira Da Costa 2015-12-27 16:35:25 UTC
Git commit 4280d9b484af5315d9522c682d73462b4b98de7a by Hugo Pereira Da Costa.
Committed on 27/12/2015 at 16:34.
Pushed by hpereiradacosta into branch 'master'.

Call updateButtonGeometry after decorationButtonsLeftChanged and decorationButtonsRightChanged
added ::update at the end of updateButtonGeometry

M  +11   -0    kdecoration/oxygendecoration.cpp
M  +1    -0    kdecoration/oxygendecoration.h

http://commits.kde.org/oxygen/4280d9b484af5315d9522c682d73462b4b98de7a
Comment 25 Till Schäfer 2015-12-27 16:44:39 UTC
(In reply to Hugo Pereira Da Costa from comment #15)
> Also, (OT) in the meanwhile, I've noticed that
> KDecoration2::DecoratedClient::widthChanged is called at every window move
> event. (without resize). 
> Should I file a separate bug report ?
Don't know if this is still an issue, but i aligns with my observation, that moving the window fixes the original bug. 

BTW: Thx 4 fixing.