D27337: [Emojier] Optimize performance

[Aleix Pol Gonzalez]

apol added a comment.


  Here's an alternative https://phabricator.kde.org/D27346

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> broulik wrote in CategoryPage.qml:71
> > but there was a bit of an empty space on the right.
> 
> If `Label` resizing is the problem. Would keeping the `Label` square and 
> using a padding `Item` around it fix the performance issue?

Yes, or just not fitting the text I guess, which is unfortunate but maybe 
necessary at the moment.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> apol wrote in CategoryPage.qml:71
> This was added so that the columns adapt to the width of the view, otherwise 
> it was very fast (as fast as without your Loader) but there was a bit of an 
> empty space on the right.

> but there was a bit of an empty space on the right.

If `Label` resizing is the problem. Would keeping the `Label` square and using 
a padding `Item` around it fix the performance issue?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> CategoryPage.qml:71
>  
> -cellWidth: width/columnsToHave
> +cellWidth: desiredSize
>  cellHeight: desiredSize

This was added so that the columns adapt to the width of the view, otherwise it 
was very fast (as fast as without your Loader) but there was a bit of an empty 
space on the right.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Aleix Pol Gonzalez]

apol added a comment.


  Right, maybe we can find a better way of doing this. Or just delaying 
resizing of the Label since it's indeed the slow part.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Chris Holland]

Zren added a comment.


  I used `Loader` as I didn't want loading glyphs to block scrollbar 
responsiveness. `GridView` reuses delegates, but I don't think it's the 
`MouseArea` + `Label` constructor that's slow. I think it's the "rendering the 
emoji glyph" in the GUI draw that is the root cause.
  
  The cell spacing code re-renders the glyph (which is a slow process), which 
is why I removed it. Though I guess we can use `anchor.centerIn`:
  
MouseArea {
id: mouse
width: emojiView.cellWidth
height: emojiView.cellHeight

Label {
anchor.centerIn: mouse
width: emojiView.desiredSize
height: emojiView.desiredSize
}
}
  
  ... to keep the cell spacing if that's a requirement.
  
  Using `fontSizeMode: Qt.VerticalFit` would make emoji's overflow 
horizontally. Various "hand + skin tone" emoji's that don't yet have a custom 
emoji are 2x wider than they are tall.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Kai Uwe Broulik]

broulik added a comment.


  I suspect `DelegateRecycler` doesn't do grid, otherwise this could have been 
an option?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[David Edmundson]

davidedmundson added a comment.


  What the original code was trying to do was distribute the whitespace among 
cells, right now if you resize to be 5.8 columns wide we want to spread that 
out, not have a gap on the white.
  
  You're right on the fonts resizing though.
  This is also a fix:
  
  - fontSizeMode: Text.Fit
  
  +fontSizeMode: Text.VerticalFit
  
  Though given we have a fixed grid size, maybe we could just set a pixelSize 
in here. That'd be even faster.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: broulik, davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, 
ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[David Edmundson]

davidedmundson added a comment.


  Delegates are already async loaded. What does nesting a loader provide on top?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: davidedmundson, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D27337: [Emojier] Optimize performance

[Nathaniel Graham]

ngraham added reviewers: apol, Plasma.
ngraham added a comment.


  Very nice.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

To: Zren, apol, #plasma
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D27337: [Emojier] Optimize performance

[Chris Holland]

Zren created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
Zren requested review of this revision.

REVISION SUMMARY
  BUG: 417454
  
  The emojier fairly slow. Launching is slow. Resizing the window is slow. 
Changing pages is slow. Scrolling is also slow.
  
  On launch, the sidebar widens from `0px`, resizing the `GridView`, which 
resizes `cellWidth` as we abuse it's the width calculation to create cell 
spacing. Resizing 1000 emoji Labels is slow. This also happens when you resize 
the window. The cpu core spikes to 100% (on a 2.8Ghz cpu).
  
  F8098605: screen-2020-02-11_18.21.04.mp4 

  
  After selecting an emoji, closing, and reopening, the recent emoji's is shown 
first (without a search box) which opens faster since it has much less work. 
However switching to the all emoji page resurfaces the problem.
  
  Scrolling is also a problem, as loading the emoji `QQC2.Label`s is slow. So 
we'll use a `Loader { asynchronous: true }`. I'm not sure why it loads them 
from bottom to top.
  
  Here's what it looks like after this patch. Resizing no longer blocks, nor 
does scrolling.
  
  F8098612: screen-2020-02-11_18.18.37.mp4 


TEST PLAN
  How I compile and run emojier by itself:
  https://gist.github.com/Zren/1d16e51199c9e47718ccfe41755d8ee2

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D27337

AFFECTED FILES
  applets/kimpanel/backend/ibus/emojier/ui/CategoryPage.qml

To: Zren
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart