Bug 344257 - idempotent ::calculateGravitation() bound to Decoration::bordersChanged in ::createDecoration()
Summary: idempotent ::calculateGravitation() bound to Decoration::bordersChanged in ::...
Status: REPORTED
Alias: None
Product: kwin
Classification: Plasma
Component: core (show other bugs)
Version: git master
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: KWin default assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-16 22:50 UTC by Thomas Lübking
Modified: 2020-11-25 08:43 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Lübking 2015-02-16 22:50:20 UTC
It calls
move(calculateGravitation(true));
move(calculateGravitation(false));

what does nothing.
What it *should* do is to perform "calculateGravitation(true)" with the *old* borders and calculateGravitation(false) with the *new* borders (alternatively calculate and move by the diff, but that's a micro-optimizing detail)

=> This requires either an API change do Decoration (to eg. pass the old borders with the signal) or to cache the borders in the core (and update them on borders changed)

Reproducible: Always
Comment 1 Martin Flöser 2015-02-17 07:09:12 UTC
passing old borders with the signal would be kind of strange. What could be done if we want to go from kdecoration API is to emit a signal aboutToChangeBorders, which allows interested parties to cache the values. Not sure whether this would help in this case.
Comment 2 Thomas Lübking 2015-02-17 13:27:52 UTC
sending old/new on changed is actually not /that/ uncommon, see eg. notably itemviews (on col/row/section changes)

Using two signals would defy the geometry update blocker (thus cause judder/flicker) - about to change could only be used to cache the present values -> dead end (it would be really stupid to update a cache to the old value right before the change is signalled and grab through current values all the time)
So, while pot. interesting in general, I don't think that would be helpful here - no.
Comment 3 Justin Zobel 2020-11-12 00:14:25 UTC
Thomas can you please test and confirm if this issue is still occurring or if this bug report can be marked as resolved. I'm setting status to "needsinfo" pending your response, please change back to "reported" or "resolved" when you respond, thanks.
Comment 4 Thomas Lübking 2020-11-12 06:32:38 UTC
I stopped using KDE ~5 years ago and also haven't really looked at the kwin code ever since then (this wasn't a user bug, I was heavily involved in kwin development)

I would suggest to check whether those code fragments still exist.
Comment 5 Justin Zobel 2020-11-12 08:49:51 UTC
Setting back to reported for investigation.
Comment 6 Vlad Zahorodnii 2020-11-25 08:40:19 UTC
(In reply to Thomas Lübking from comment #4)
> I stopped using KDE ~5 years ago and also haven't really looked at the kwin
> code ever since then (this wasn't a user bug, I was heavily involved in kwin
> development)
> 
> I would suggest to check whether those code fragments still exist.

We still have it. As for this bug, we could either pass the old borders as a signal arg or cache the last frame border extents in the bordersChanged() slot.
Comment 7 Vlad Zahorodnii 2020-11-25 08:43:15 UTC
by cache, I mean

    connect(decoration, &Decoration::bordersChanged, this, [this] {
        move(calculateGravity(true, m_cachedBorders)); // undo
        move(calculateGravity(false, decoration()->borders()); // redo
        m_cachedBorders = decoration()->borders();
    });