Icon image loading is synchronous

[Albert Astals Cid]

The Image {} in Icon*.qml is not setting asynchronous to true.

I would suggest to set it to true so the loading is not blocking the main thread.
If you don't want to set it to async by default maybe you can add an alias so we can set it externally?

What do you think?

[Zsombor Egri (zsombi)]

Yes, agree on this. However, to avoid the other parties, who want to load synchronously, the best will be to expose the property to the Icon API.

[Zsombor Egri (zsombi)]

The default behaviour will be true, so backwards compatibility is kept.

[Andrea Bernabei (faenil)]

does it make sense to keep backward compatibility in this case?

it sounds more like just a performance issue here...or is there any strong usecase where you absolutely need the Image to load syncronously?

[Zsombor Egri (zsombi)]

Andrea, whenever you add a property you should not change the previous behaviour of the component, right? So yes, it has to be kept as Image.asynchronous default is.

[Andrea Bernabei (faenil)]

it depends if the previous behaviour was a bug. And this case, I could argue it is...unless we have strong reasons for having it sync by default

[Zsombor Egri (zsombi)]

No, the previous behaviour wasn't a bug. Neither Image's default behaviour is a bug. Therefore we must obey the default behaviour and default it to whatever Image's default is, which is synchronous.

[Andrea Bernabei (faenil)]

Icon is our component, not Qt's, so we have the power to change the default values.

Can you please argue why sync should be a default instead of async? I'm just looking for a valid point here, otherwise we just get smoothness penalty for no real reason.

[Zsombor Egri (zsombi)]

See comment #2. And let me come with an example Qt screwed up from one version to the other.

MultiPointTouchArea. The default component was handling only touch events, not taking care of the Mouse events. In order to handle mouse event you had to put a MouseArea inside of it, and then one handled the touch, the other the mouse events. At some point upstream introduced the mouseEnabled property with a default value of true. From that point the MouseAreas placed inside the MultiPointTouchAreas were not getting any mouse events, as the default behaviour of the MultiPointTouchArea changed. This is not an API break, but a behavioural one.

We are in the same situation. We must introduce the API with backwards behavioural compatibility, so UI won't look like tiles coming up one after other just because the icon loading suddenly became asynchronous. Beside, the property will be exposed, so I do not understand your opposed to keep backwards behavioural compatibility.

And btw, the Scrollbars dump loads of binding loop warnings with default asynchronous behaviour...

[Andrea Bernabei (faenil)]

Of course it is a behavioural break, I was just wondering whether it has many drawbacks, or it was actually worth it because of the improvements in smoothness it could bring.

If there's an issue with the Scrollbar, that's a separate bug ;) but yes, I get your point that the same may happen with other components that are assuming sync image loading...but maybe we should just fix those? ;)

just my 2 cents

[Launchpad Janitor (janitor)]

This bug was fixed in the package ubuntu-ui-toolkit - 1.3.1872+16.04.20160308-0ubuntu1

---------------
ubuntu-ui-toolkit (1.3.1872+16.04.20160308-0ubuntu1) xenial; urgency=medium

  [ Timo Jyrinki ]
  * Fix gles unit test skipping.
  * Add s390x to the unit test skipping architectures
  * UITK test plan - wily -> xenial. Fixes LP: #1544490

  [ Christian Dywan ]
  * Print errors when QuickUtils fails to create a component.
  * unit tests shouldn't use deprecated Dialog.callera
  * Correctly count only test case results. The result= value is used in suites
    and results.
  * Skip tst_pagehead_visible flakes on non-arm. Fixes LP: #1534651
  * Skip flaky tst_mousefilterTest::doubleClicked. Fixes LP: #1542215
  * Suppress click signal if releasing outside of ListItem. Fixes LP: #1541148.
  * Use gdb in runtest.sh to produce backtraces.
  * Arrow keys change value of a Slider. Fixes LP: #1523824.
  * Only use Maliit when enabled explicitly in the environment

  [ Oliver Tilloy ]
  * Do not try to assign to non-existent property "activeFocusOnPress".
    Fixes LP: #1532953
  * Bubble up ESC key press event if there is no popover to close.
    Fixes LP: #1546627

  [ Loïc Molinari ]
  * Added private items and nodes for the new component styles.
    Fixes LP: #1523836
  * Made use of new private Frame item for the focusing.
  * Fixed performance monitor crash because of a NULL timer pointer dereference.
    Fixes LP: #1546986
  * Fixed performance monitor dangling pointer crash. Fixes LP: #1546984
  * [ProportionalShape] Ensured width/height ratio is correct with default values.
    Fixes LP: #1546546
  * Added a workaround to prevent a crash while changing the QPA scale factor.

  [ Florian Boucault ]
  * Always on performance monitor that logs frames that too long to render.
  * MainView: proceed to selecting the theme automatically at startup too.
    Fixes LP: #1535819
  * MainView: when no gradient color is needed for the background, rely on
    QQuickWindow's GL clear color. Lessens overdraw considerably for most
    apps. Fixes LP: #1439133.
  * Panel: MathUtils used without import prefix was leading to undefined
    reference.
  * Label: use native rendering on low dpi screens (GRID_UNIT_PX <= 10) for
    sharper looking text.
  * Gallery: added palette browser to Colors page.

  [ Tim Peeters ]
  * Support scrolling in ListViews with horizontal orientation to the
    qquicklistview CPO.
  * Add background to AppHeader and remove contents clipping from MainView.
    Fixes LP: #1531016, LP: #1531457.
  * Hide AppHeader when using AdaptivePageLayout. Fixes bug 1531871.
    Fixes LP: #1531871.
  * Implement horizontal flicking in the flickable autopilot CPO.
  * Re-order to list of pages in the gallery to be alphabetical.
  * Fix the autopilot failures introduced with the horizontal scrolling in
    the Flickable CPO.
  * Update documentation for MainView, Page, AdaptivePageLayout to use the new
    PageHeader in all examples. Deprecate old properties. Fixes LP: #1540574
  * Add 'animate' property to new internal AppHeaderBase, and do not show a
    header animation when starting an app without header.
    Fixes LP: #1518002, LP: #1524901.
 ...