Review Request 114275: Use a QMap as source of EmoticonTextEditItem elements

[David Edmundson]

> On Dec. 3, 2013, 7:23 p.m., David Edmundson wrote:
> > app/emoticon-text-edit-selector.cpp, line 104
> > <https://git.reviewboard.kde.org/r/114275/diff/1/?file=222359#file222359line104>
> >
> >     Could we just replace this first for loop here with:
> >     
> >     Q_FOREACH(const QString &emoticonPath, qSort(list.keys())) {
> >        new EmoticonItem(list[emoticonPath].first(), emoticonPath, d->listEmoticon);
> >     }
> >     
> >     rather than looping through everything twice?
> >     
> >     Might be easier to read.
> >     
> >     (note I've not tested this)

Just so you know, I'm expecting a reply :)

Either:
 "yes, that looks like a good solution which solves the problem" or "No, my original patch is better because of X"


- David


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


On Dec. 3, 2013, 3:37 p.m., Giuseppe Calà wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/114275/
> -----------------------------------------------------------
> 
> (Updated Dec. 3, 2013, 3:37 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-text-ui
> 
> 
> Description
> -------
> 
> First of all, two assumptions:
> 1. I know that this code should be supplied upstream to kdelibs but ktp-text-ui's code is smaller and easier, for me, to manage.
> 2. This patch tries to fix what in my opinion is an usability issue.
> 
> Ktp-text-ui uses a QHash from KEmoticonsTheme::emoticonsMap() to build emoticons' QListWidget. The problem is that items in a QHash are arbitrarily ordered so the resulting view is messed up. With this patch the returned QHash is instead used to build a QMap, whose elements are ordered by key, and use this one as source for EmoticonTextEditItem. The final result is a view where emoticons are ordered by their filename.
> 
> 
> Diffs
> -----
> 
>   app/emoticon-text-edit-selector.cpp 1fad99b 
> 
> Diff: https://git.reviewboard.kde.org/r/114275/diff/
> 
> 
> Testing
> -------
> 
> Tested with Google Hangout Emojis Complete theme from kde-look.org:
> 
> Before: http://wstaw.org/m/2013/12/03/emoticons-view-before.png
> After: http://wstaw.org/m/2013/12/03/emoticons-view-after.png
> 
> 
> Thanks,
> 
> Giuseppe Calà
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20131227/5ac0197b/attachment-0001.html>