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

[Alexandr Akulich]

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

(Updated Sept. 20, 2016, 5:46 p.m.)


Status
--

This change has been marked as submitted.


Review request for Telepathy.


Changes
---

Submitted with commit cbd7bb27f5caa776c3a41bcb5d35bcf4cb2376b2 by Alexandr 
Akulich to branch master.


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

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

[Aleix Pol Gonzalez]

> On Sept. 20, 2016, 12:06 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 222
> > 
> >
> > 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 (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.
> 
> Alexandr Akulich wrote:
> 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!

Ok, do that.


- Aleix


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


On Sept. 20, 2016, 11:51 a.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, 11:51 a.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
> 
>

Re: 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
> > 
> >
> > 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 (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
> 
>

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

[Aleix Pol Gonzalez]

> On Sept. 20, 2016, 12:06 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 222
> > 
> >
> > 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 (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?

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.


- Aleix


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


On Sept. 20, 2016, 11:51 a.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, 11:51 a.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
> 
>

Re: 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
> > 
> >
> > 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 (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

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?


- 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
> 
>

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

[Aleix Pol Gonzalez]

> On Sept. 20, 2016, 12:06 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 222
> > 
> >
> > 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 (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.

You can use reverse iterators:
http://doc.qt.io/qt-5/qvector.html#crbegin


- Aleix


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


On Sept. 20, 2016, 11:51 a.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, 11:51 a.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
> 
>

Re: 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
> > 
> >
> > 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.

Actually I call ```const T (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.


- 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
> 
>

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

[Aleix Pol Gonzalez]

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




KTp/Declarative/messages-model.cpp (line 221)


I'd remove the comment, I'd say it's obvious that this needs to be iterated 
backwards.



KTp/Declarative/messages-model.cpp (line 222)


Use iterators?


- Aleix Pol Gonzalez


On Sept. 20, 2016, 11:51 a.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, 11:51 a.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
> 
>