Review Request 128954: [KTp/Declarative/MessagesModel] Implemented message sort by sent timestamp

[Alexandr Akulich]

> On Sept. 20, 2016, 3:06 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 222
> > <https://git.reviewboard.kde.org/r/128954/diff/1/?file=477161#file477161line222>
> >
> >     Use iterators?
> 
> Alexandr Akulich wrote:
>     How to get an index from iterator?
> 
> Aleix Pol Gonzalez wrote:
>     You can count in parallel without calling `QList::operator[]` on the index.
> 
> Alexandr Akulich wrote:
>     Actually I call ```const T &at(int i) const;```. And what is the reason to use iterators, if you still suggest to use a counter? Do you actually think that
>     ```
>         int i = d->messages.count() - 1;
>         for (auto it = d->messages.constEnd(); it != d->messages.constBegin(); --it, --i) {
>             if (sentTimestamp > it->message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>             // Or do you suggest to place --i; here?
>         }
>     ```
>     is more readable, than plain
>     ```
>         for (int i = d->messages.count() - 1; i >= 0; --i) {
>             if (sentTimestamp > d->messages.at(i).message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>         }
>     ```
>     ?
>     
>     IMO it is a common practice to use iterators if you don't need index (and just use the index otherwise). It is a bit more optimal and I don't made an error in the condition.
> 
> Aleix Pol Gonzalez wrote:
>     You can use reverse iterators:
>     http://doc.qt.io/qt-5/qvector.html#crbegin
> 
> Alexandr Akulich wrote:
>     I know about the reverse methods and it is not any better without some reverse adaptor in QList.
>     
>         int i = d->messages.count() - 1;
>         for (auto it = d->messages.crbegin(); it != d->messages.crend(); ++it, --i) {
>             if (sentTimestamp > it->message.time()) {
>                 newMessageIndex = i;
>                 break;
>             }
>             // Or do you suggest to place --i; here?
>         }
>         
>     Why it is better? It reads worse and it looks like "iterators for the iterators" without a real benefits. It does not save us from an error, because we need the (reverse) counter. Do we have a coding convention for this case?
> 
> Aleix Pol Gonzalez wrote:
>     I'd say in general the coding convention is to use iterators whenever possible, since we can ensure the access to the random object will be optimal. If you really feel like this is worse, feel free to use something else. I didn't come here to play sargeant.

I'm sorry if I sounded rude or unfriendly.

Iterators can indeed save us from the assert in QList::at(), but I think that in this particular case the code will be more complex, harder to understand and thus error-prone: we use reverse iterators, which we have to increase and at the same time decrease the counter. If there would be no beginInsertRows() call, I would be all for use iterators.

In this case we can follow the KISS rule without performance penalty.

By the way, I can not find anything related to iterators usage in "Qt Coding Style", "Qt Coding Conventions" or "Frameworks Coding Style" (which seems to be a successor of Kdelibs Coding Style). The only rule is "Don't mix const and non-const iterators." Probably we need to update the convention(s) with C++11 things.

I will remove the "obvious" comment and push this change, if you agree. I would like to upload one may be "more discussable" change, which depends on this one.

And anyway, thanks for the feedback!


- Alexandr


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128954/#review99301
-----------------------------------------------------------


On Sept. 20, 2016, 2:51 p.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128954/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 2:51 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> Implemented message sort by sent timestamp (if available).
> 
> This fixes order of scrollback messages.
> 
> 
> Diffs
> -----
> 
>   KTp/Declarative/messages-model.cpp dc1088c 
> 
> Diff: https://git.reviewboard.kde.org/r/128954/diff/
> 
> 
> Testing
> -------
> 
> Works, fixes the issue.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20160920/dc12f76b/attachment.html>