date:20160920

Re: Review Request 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

2016-09-20 Thread Alexandr Akulich


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

(Updated Sept. 20, 2016, 2:37 p.m.)


Status
--

This change has been marked as submitted.


Review request for Telepathy.


Changes
---

Submitted with commit 2706d8d60cedcdcc31067122c909797d506ff629 by Alexandr 
Akulich to branch master.


Repository: ktp-common-internals


Description
---

We actually story the index, so we should not just reference it.


Diffs
-

  KTp/Declarative/messages-model.cpp 6823574 

Diff: https://git.reviewboard.kde.org/r/128961/diff/


Testing
---

It compiles.


Thanks,

Alexandr Akulich

Re: Review Request 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

2016-09-20 Thread Aleix Pol Gonzalez


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


Ship it!




Ship It!

- Aleix Pol Gonzalez


On Sept. 20, 2016, 3:34 p.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128961/
> ---
> 
> (Updated Sept. 20, 2016, 3:34 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> We actually story the index, so we should not just reference it.
> 
> 
> Diffs
> -
> 
>   KTp/Declarative/messages-model.cpp 6823574 
> 
> Diff: https://git.reviewboard.kde.org/r/128961/diff/
> 
> 
> Testing
> ---
> 
> It compiles.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

2016-09-20 Thread Aleix Pol Gonzalez


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


Ship it!




Ship It!

- Aleix Pol Gonzalez


On Sept. 20, 2016, 3:27 p.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128960/
> ---
> 
> (Updated Sept. 20, 2016, 3:27 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> Sent and received messages can be received again as scrollback and before 
> this patch it leads to messages duplication.
> E.g.: server sent last 20 messages on connection to a group chat, the local 
> user has a network issue and thus got disconnected and connected back with 
> messaging window kept open.
> 
> We already have a table for sent messages and this patch:
> 1) adds insertion of received messages into the same table,
> 2) uses the table to ignore already sent/received messages.
> 
> In future we can also use the table to implement message correction (see 
> XEP-0308, Message_Header_Key "supersedes" and so on).
> 
> 
> Diffs
> -
> 
>   KTp/Declarative/messages-model.cpp 6823574 
> 
> Diff: https://git.reviewboard.kde.org/r/128960/diff/
> 
> 
> Testing
> ---
> 
> Sent and received messages are not duplicated on scrollback received.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Review Request 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

2016-09-20 Thread Alexandr Akulich


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

Review request for Telepathy.


Repository: ktp-common-internals


Description
---

We actually story the index, so we should not just reference it.


Diffs
-

  KTp/Declarative/messages-model.cpp 6823574 

Diff: https://git.reviewboard.kde.org/r/128961/diff/


Testing
---

It compiles.


Thanks,

Alexandr Akulich

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

2016-09-20 Thread Alexandr Akulich


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

(Updated Sept. 20, 2016, 6:27 p.m.)


Review request for Telepathy.


Changes
---

Fixed modelIndex variable type.


Repository: ktp-common-internals


Description
---

Sent and received messages can be received again as scrollback and before this 
patch it leads to messages duplication.
E.g.: server sent last 20 messages on connection to a group chat, the local 
user has a network issue and thus got disconnected and connected back with 
messaging window kept open.

We already have a table for sent messages and this patch:
1) adds insertion of received messages into the same table,
2) uses the table to ignore already sent/received messages.

In future we can also use the table to implement message correction (see 
XEP-0308, Message_Header_Key "supersedes" and so on).


Diffs (updated)
-

  KTp/Declarative/messages-model.cpp 6823574 

Diff: https://git.reviewboard.kde.org/r/128960/diff/


Testing
---

Sent and received messages are not duplicated on scrollback received.


Thanks,

Alexandr Akulich

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

2016-09-20 Thread Alexandr Akulich



> On Sept. 20, 2016, 6:13 p.m., Aleix Pol Gonzalez wrote:
> > KTp/Declarative/messages-model.cpp, line 245
> > 
> >
> > this shouldn't be a reference, as you're actually storing it.

Copied from the first usage:
https://github.com/KDE/ktp-common-internals/blob/master/KTp/Declarative/messages-model.cpp#L257

But I agree, this is a bug which I missed.

I will add another PR to fix it in the first place and update this PR too.


- Alexandr


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


On Sept. 20, 2016, 6:09 p.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128960/
> ---
> 
> (Updated Sept. 20, 2016, 6:09 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> Sent and received messages can be received again as scrollback and before 
> this patch it leads to messages duplication.
> E.g.: server sent last 20 messages on connection to a group chat, the local 
> user has a network issue and thus got disconnected and connected back with 
> messaging window kept open.
> 
> We already have a table for sent messages and this patch:
> 1) adds insertion of received messages into the same table,
> 2) uses the table to ignore already sent/received messages.
> 
> In future we can also use the table to implement message correction (see 
> XEP-0308, Message_Header_Key "supersedes" and so on).
> 
> 
> Diffs
> -
> 
>   KTp/Declarative/messages-model.cpp 6823574 
> 
> Diff: https://git.reviewboard.kde.org/r/128960/diff/
> 
> 
> Testing
> ---
> 
> Sent and received messages are not duplicated on scrollback received.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

2016-09-20 Thread Aleix Pol Gonzalez


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


Fix it, then Ship it!




I was not aware of this QPersistentModel hash, should probably look into it in 
the future.


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


this shouldn't be a reference, as you're actually storing it.


- Aleix Pol Gonzalez


On Sept. 20, 2016, 3:09 p.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128960/
> ---
> 
> (Updated Sept. 20, 2016, 3:09 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> Sent and received messages can be received again as scrollback and before 
> this patch it leads to messages duplication.
> E.g.: server sent last 20 messages on connection to a group chat, the local 
> user has a network issue and thus got disconnected and connected back with 
> messaging window kept open.
> 
> We already have a table for sent messages and this patch:
> 1) adds insertion of received messages into the same table,
> 2) uses the table to ignore already sent/received messages.
> 
> In future we can also use the table to implement message correction (see 
> XEP-0308, Message_Header_Key "supersedes" and so on).
> 
> 
> Diffs
> -
> 
>   KTp/Declarative/messages-model.cpp 6823574 
> 
> Diff: https://git.reviewboard.kde.org/r/128960/diff/
> 
> 
> Testing
> ---
> 
> Sent and received messages are not duplicated on scrollback received.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

Review Request 128960: [KTp/Declarative/MessagesModel] Used token to prevent incoming message duplication

2016-09-20 Thread Alexandr Akulich


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

Review request for Telepathy.


Repository: ktp-common-internals


Description
---

Sent and received messages can be received again as scrollback and before this 
patch it leads to messages duplication.
E.g.: server sent last 20 messages on connection to a group chat, the local 
user has a network issue and thus got disconnected and connected back with 
messaging window kept open.

We already have a table for sent messages and this patch:
1) adds insertion of received messages into the same table,
2) uses the table to ignore already sent/received messages.

In future we can also use the table to implement message correction (see 
XEP-0308, Message_Header_Key "supersedes" and so on).


Diffs
-

  KTp/Declarative/messages-model.cpp 6823574 

Diff: https://git.reviewboard.kde.org/r/128960/diff/


Testing
---

Sent and received messages are not duplicated on scrollback received.


Thanks,

Alexandr Akulich

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

2016-09-20 Thread 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

2016-09-20 Thread 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

2016-09-20 Thread 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

2016-09-20 Thread 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 128867: [KTp/Message] Direction of received message now depends on sender

2016-09-20 Thread Alexandr Akulich


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

(Updated Sept. 20, 2016, 7:05 a.m.)


Status
--

This change has been marked as submitted.


Review request for Telepathy.


Changes
---

Submitted with commit 431db458de1417764ecdef00974fca4c412e19f8 by Alexandr 
Akulich to branch master.


Repository: ktp-common-internals


Description
---

If the sender is selfContact, then the direction should be LocalToRemote even 
for ReceivedMessages.

With this fix Scrollback feature in ktp-text-ui starts to partially work.

Remaining issues (with scrollback):
- If ktp-proxy is on, then sender of received message forced to the channel 
target, so all received messages would be "incoming";
- Messages in TextUi appears in order of receiving. This means that if 
scrollback received from the newer to older messages, then the older message 
would be showed at the end (like if it was been just sent).


Diffs
-

  KTp/message.cpp 566241e 

Diff: https://git.reviewboard.kde.org/r/128867/diff/


Testing
---

Compiles, runs. Fixes scrollback for 40% (30% and 30% are broken ktp-proxy and 
buggy ktp-text-ui, which shows messages in straight instead of chronological 
message order).


Thanks,

Alexandr Akulich

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

2016-09-20 Thread 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 128847: [ktp-common-internals] [debugger] Split logic and UI

2016-09-20 Thread Aleix Pol Gonzalez


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



I'd say that if these changes fit you, they should go in. I honestly never used 
ktp-debugger for more than 1 minute straight.

- Aleix Pol Gonzalez


On Sept. 6, 2016, 4:54 p.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128847/
> ---
> 
> (Updated Sept. 6, 2016, 4:54 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> The main goal of this change is to split logic and UI parts
> 
> This is the first step in direction to debugger, which:
> 1) works with any Telepathy process with DebugInterface support;
> 2) detects new processess "on fly";
> 3) has no hardcoded services;
> 4) shows one process just once, independently of number of dbus services, 
> registered by the process.
> 
> The change also opens a way to a QML-based UI at some point in future.
> 
> Questionable thing is the "TelepathyProcess" class name.
> TelepathyService does not fit, because:
> 1) Single process can expose a number of services (e.g. MissionControl),
> 2) The debug interface is applicable to any telepathy application, including 
> clients, so word "Service" (which is not associated with clients) would 
> mislead.
> 
> 
> I uploaded a draft of "second step" to my scratch repo:
> https://quickgit.kde.org/?p=scratch%2Fakulichalexandr%2Fktp-common-internals.git=commitdiff=7e07b65f330d85527c9a6b014154527f7e3e7c01=db202a7143be88db37e056913a88992fe7ce507d
> 
> I will make a ReviewRequest with the second part on this (split) commit 
> landed.
> 
> 
> Diffs
> -
> 
>   tools/debugger/CMakeLists.txt e35de89 
>   tools/debugger/debug-message-view.h ae745db 
>   tools/debugger/debug-message-view.cpp ea09d79 
>   tools/debugger/main-window.cpp 490f803 
>   tools/debugger/telepathy-process.h PRE-CREATION 
>   tools/debugger/telepathy-process.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/128847/diff/
> 
> 
> Testing
> ---
> 
> Works as previously.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

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

2016-09-20 Thread 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 128867: [KTp/Message] Direction of received message now depends on sender

2016-09-20 Thread Aleix Pol Gonzalez


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


Ship it!




Ship It!

- Aleix Pol Gonzalez


On Sept. 9, 2016, 12:18 a.m., Alexandr Akulich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128867/
> ---
> 
> (Updated Sept. 9, 2016, 12:18 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> ---
> 
> If the sender is selfContact, then the direction should be LocalToRemote even 
> for ReceivedMessages.
> 
> With this fix Scrollback feature in ktp-text-ui starts to partially work.
> 
> Remaining issues (with scrollback):
> - If ktp-proxy is on, then sender of received message forced to the channel 
> target, so all received messages would be "incoming";
> - Messages in TextUi appears in order of receiving. This means that if 
> scrollback received from the newer to older messages, then the older message 
> would be showed at the end (like if it was been just sent).
> 
> 
> Diffs
> -
> 
>   KTp/message.cpp 566241e 
> 
> Diff: https://git.reviewboard.kde.org/r/128867/diff/
> 
> 
> Testing
> ---
> 
> Compiles, runs. Fixes scrollback for 40% (30% and 30% are broken ktp-proxy 
> and buggy ktp-text-ui, which shows messages in straight instead of 
> chronological message order).
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

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

2016-09-20 Thread 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

2016-09-20 Thread 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
> 
>

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

2016-09-20 Thread Alexandr Akulich


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

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 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

Re: Review Request 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

Review Request 128961: [KTp/Declarative/MessagesModel] Fixed allocation of modelIndex variable in onMessageSent()

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

Re: Review Request 128960: [KTp/Declarative/MessagesModel] Use message token to prevent message duplication on scrollback received

Review Request 128960: [KTp/Declarative/MessagesModel] Used token to prevent incoming message duplication

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

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

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

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

Re: Review Request 128867: [KTp/Message] Direction of received message now depends on sender

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

Re: Review Request 128847: [ktp-common-internals] [debugger] Split logic and UI

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

Re: Review Request 128867: [KTp/Message] Direction of received message now depends on sender

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

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

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

20 matches

Site Navigation

Mail list logo

Footer information