Wunderfitz / harbour-fernschreiber

Fernschreiber is a Telegram client for Sailfish OS
GNU General Public License v3.0
43 stars 34 forks source link

Prepare another optimization round #198

Open jgibbon opened 4 years ago

jgibbon commented 4 years ago

I initially only thought about optimizing OverviewPage, because that didn't get much love, yet. But then some ChatPage issues "popped up", as well.

As I am not able to assess all of those points (not very familiar with how/why some of these things were done exactly, especially c++ interaction), I'd appreciate some feedback (or even implementation help later on) at least on the less obvious ones marked with :question::question:. They are numbered to make referencing a tiny bit easier.

1. OverviewPage

2. ChatPage

2.2 MessageListViewItem

3. visible/opacity bindings

Some bindings are checked twice instead of just checking on visible and referring to that, or visible even rechecks on every change of the opacity Behavior:

jgibbon commented 4 years ago

Prioritization:

The only really bad thing that I can't handle without some context is 1.3 from the OverviewPage:

1.3 :question::question: Is redrawModel really, really needed? It recreates all rendered delegates AFAICT, which is a bit heavy & weird on init. Almost all other init issues' impact gets doubled by this one, I think.

The other things either aren't really bad or I can fix them myself.

monich commented 4 years ago

Slightly off topic but since opacity animation was referenced.... There're two ways to bind opacity and visibility. This:

visible: opacity > 0
opacity: needToDisplay ? 1 : 0

and this:

opacity: visible ? 1 : 0
visible: needToDisplay

Both make sense but behave differently with respect to opacity animation. In the first case visible: opacity > 0 the item's opacity is being animated both ways, when it's appearing and when it's disappearing. In the second case opacity would be animated only when the item is appearing on the screen. When visible becomes false, the item disappears immediately regardless of its opacity.

I typically use the first variant by default.

And yes, it's a good idea to set visible to false for the items which are not needed, to completely remove them from the event processing chain.

monich commented 4 years ago

I would also look towards using enums (e.g. for chat/message types) and compare numbers rather than strings. C++ part (exposing more enums and associated roles from the models) need to be done first, and in that sense doesn't interfere at all with QML-only optimization.

Using TDLibFile can remove some signal processing from QML and push it to native code which should be good for performance.

jgibbon commented 4 years ago

Not off topic at all, that is a very good point! I'll make sure to skip 3.1, since it's very much intended.

jgibbon commented 4 years ago

Using TDLibFile is also a good addition.