Bug 455789 - "Don't override the leftPadding or rightPadding on a SwipeListItem!" message shown for SwipeListItem without leftPadding or rightPadding overridden
Summary: "Don't override the leftPadding or rightPadding on a SwipeListItem!" message ...
Status: RESOLVED FIXED
Alias: None
Product: frameworks-kirigami
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: Master
Platform: Other Linux
: NOR normal
Target Milestone: Not decided
Assignee: Marco Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-22 14:49 UTC by Nate Graham
Modified: 2022-10-18 18:00 UTC (History)
4 users (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 Nate Graham 2022-06-22 14:49:02 UTC
Launching Discover and navigating to the Settings page shows this error:

> qml: Don't override the leftPadding or rightPadding on a SwipeListItem!
> This makes it impossible for me to adjust my layout as I need to for various usecases.
> I'll try to fix the mistake for you, but you should remove your overrides from your app's code entirely.
> If I can't fix the paddings, I'll fall back to a default layout, but it'll be slightly incorrect and lacks
> adaptations needed for touch screens and right-to-left languages, among other things.

However the SwipeListItem implementation in Discover does not override its leftPadding or rightPadding properties. See for yourself: https://invent.kde.org/plasma/discover/-/blob/master/discover/qml/SourcesPage.qml#L177
Comment 1 ratijas 2022-06-22 21:13:23 UTC
The new code mixes declarative bindings with imperative function with side-effects (console warn). The two don't play nicely together, as the order of signals propagation is not defined. However, as with most UI types, some validation could've been pushed to the next "polish" step when all properties are expected to settle down on their final values. In QML we don't have that much control over API to intercept polish events, but at least we have one last resort: Qt.callLater() function -- which should be used for that validation step. In addition, it would also de-duplicate message calls scheduled during a single event loop cycle.
Comment 2 Nate Graham 2022-07-18 14:36:59 UTC
Getting reports of this warning erroneously being printed for all SwipeListItems.
Comment 3 bart 2022-07-21 11:26:51 UTC
(In reply to ratijas from comment #1)
> The new code mixes declarative bindings with imperative function with
> side-effects (console warn). The two don't play nicely together, as the
> order of signals propagation is not defined.

I'm pretty sure the propagation in this particular case is well-defined.  And it's defined in such a way that that warning/side-effect will always happen:
In controls/templates/SwipeListItem.qml, listItem has leftPadding and rightPadding which require overlayLoader.paddingOffset.  This will cause overlayLoader to be created in order to evaluate overlayLoader.paddingOffset.  This in turn will trigger the validate function; however, at that point listItem.leftPadding and/or listItem.rightPadding have not yet been set since this whole chain of events has been set in motion in order to set those paddings in the first place.  Hence, the warning is printed and the binding is forced even before it has been set the very first time.
Comment 4 Aleix Pol 2022-10-18 18:00:18 UTC
Git commit b6980005ab3878747c27c5e7265b442188953a7c by Aleix Pol.
Committed on 18/10/2022 at 16:59.
Pushed by apol into branch 'master'.

SwipeListItem: Make sure we only show the aggressive warning when it's due

I've seen at times that the warning would show in Discover despite the
conditions explained in the text would not be matched. This is because
of how Repeater works and ultimately the size here.

Instead of keeping it in a property that needs to keep being updated, do
it in a local function so at least we are calculating it on the spot
rather than using the last update of the property.

M  +6    -5    src/controls/templates/SwipeListItem.qml

https://invent.kde.org/frameworks/kirigami/commit/b6980005ab3878747c27c5e7265b442188953a7c