[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 118879: Revert to old shell script code for parsing rgb.txt
From: Aurélien Gâteau <agateau () kde ! org>
Date: 2014-06-24 8:27:33
Message-ID: 20140624082733.7396.65058 () probe ! kde ! org
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On June 23, 2014, 10:28 a.m., Aurélien Gâteau wrote:
> > Out of curiosity: why is it necessary to skip colors with spaces?
>
> Alexander Potashev wrote:
> I guess it's a heuristic to remove duplicate entries for same {R,G,B}. However, it \
> doesn't work perfectly, e.g. we have two entries "navy" and "NavyBlue" for the same \
> RGB={0, 0, 128}.
> A better algorithm would be to group all lines in rgb.txt by RGB and then choose \
> one text from each group somehow, may be take the shortest one.
> More important, we are now talking about Messages.sh which only populates \
> translation templates with strings. The rgb.txt file parser used at run-time is \
> elsewhere: see KColorTable::readNamedColor() in \
> kdelibs4support/src/kdeui/kcolordialog.cpp
I see. Since it's in kdelibs4support, I don't think we should change the C++ code \
there.
I rewrote the shell line because I found it difficult to read. I thought removing \
color with spaces was a bug but I should have looked at the code using those strings. \
The awk script can be adjusted to skip names with spaces by only outputting the name \
if there are exactly 4 fields (a name with a space would result in 5 fields). This \
can be done like this:
awk '$1 ~ "[0-9]+" && $4 !~ "gr[ae]y" && NF == 4 { print $4 }' kdeui/rgb.txt
Use whichever version you prefer, I'd just ask adding a comment on top of the shell \
code explaining those strings are used by KColorTable::readNamedColor(), from \
src/kdeui/kcolordialog.cpp.
- Aurélien
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118879/#review60754
-----------------------------------------------------------
On June 22, 2014, 2:30 p.m., Alexander Potashev wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118879/
> -----------------------------------------------------------
>
> (Updated June 22, 2014, 2:30 p.m.)
>
>
> Review request for KDE Frameworks and Aurélien Gâteau.
>
>
> Repository: kdelibs4support
>
>
> Description
> -------
>
> The code using AWK did not properly throw away entries containing
> spaces.
>
> The code inserted in this commit was used successfully in KDE SC 4, see
> kdelibs/kdeui/colors/Messages.sh
>
>
> Diffs
> -----
>
> src/Messages.sh 9b918fa
>
> Diff: https://git.reviewboard.kde.org/r/118879/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexander Potashev
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118879/">https://git.reviewboard.kde.org/r/118879/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On June 23rd, 2014, 10:28 a.m. CEST, <b>Aurélien \
Gâteau</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Out of curiosity: why is it necessary to skip colors with spaces?</pre> \
</blockquote>
<p>On June 24th, 2014, 2:15 a.m. CEST, <b>Alexander Potashev</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I guess it's a \
heuristic to remove duplicate entries for same {R,G,B}. However, it doesn't work \
perfectly, e.g. we have two entries "navy" and "NavyBlue" for the \
same RGB={0, 0, 128}.
A better algorithm would be to group all lines in rgb.txt by RGB and then choose one \
text from each group somehow, may be take the shortest one.
More important, we are now talking about Messages.sh which only populates translation \
templates with strings. The rgb.txt file parser used at run-time is elsewhere: see \
KColorTable::readNamedColor() in kdelibs4support/src/kdeui/kcolordialog.cpp</pre> \
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I see. Since it's in \
kdelibs4support, I don't think we should change the C++ code there.
I rewrote the shell line because I found it difficult to read. I thought removing \
color with spaces was a bug but I should have looked at the code using those strings. \
The awk script can be adjusted to skip names with spaces by only outputting the name \
if there are exactly 4 fields (a name with a space would result in 5 fields). This \
can be done like this:
awk '$1 ~ "[0-9]+" && $4 !~ "gr[ae]y" && \
NF == 4 { print $4 }' kdeui/rgb.txt
Use whichever version you prefer, I'd just ask adding a comment on top of the \
shell code explaining those strings are used by KColorTable::readNamedColor(), from \
src/kdeui/kcolordialog.cpp.</pre> <br />
<p>- Aurélien</p>
<br />
<p>On June 22nd, 2014, 2:30 p.m. CEST, Alexander Potashev wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KDE Frameworks and Aurélien Gâteau.</div>
<div>By Alexander Potashev.</div>
<p style="color: grey;"><i>Updated June 22, 2014, 2:30 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs4support
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">The code using AWK did not properly throw away entries containing \
spaces.
The code inserted in this commit was used successfully in KDE SC 4, see
kdelibs/kdeui/colors/Messages.sh</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/Messages.sh <span style="color: grey">(9b918fa)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118879/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic