[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kwrite-devel
Subject:    Re: Review Request: Python syntax string highlighting fixes
From:       "Matthew Woehlke" <mw_triad () users ! sourceforge ! net>
Date:       2009-09-08 22:50:36
Message-ID: 20090908225036.11289.95456 () localhost
[Download RAW message or body]



> On 2009-09-08 17:52:43, Matthew Woehlke wrote:
> > /trunk/KDE/kdelibs/kate/syntax/data/python.xml, line 382
> > <http://reviewboard.kde.org/r/1513/diff/2/?file=11087#file11087line382>
> > 
> > You do realize that by default this is the same as dsKeyword?
> 
> wrote:
> Operators are not keywords.

No, they are closer to dsOther. But  «defStyleNum="dsNormal" bold="1" » will *appear* \
the same as dsKeyword in the default configuration, so "Operator" will look like \
dsKeyword to people that have not changed dsKeyword to something other than the \
default 'the same as dsNormal, except bold'.


> On 2009-09-08 17:52:43, Matthew Woehlke wrote:
> > /trunk/KDE/kdelibs/kate/syntax/data/python.xml, line 396
> > <http://reviewboard.kde.org/r/1513/diff/2/?file=11087#file11087line396>
> > 
> > I realize you didn't change this, but there is a style for 'Base N Integers' (I \
> > forget the "dsXX" name offhand) that should probably be used for this, also for \
> > "Octal" and maybe "Complex" as well (or maybe dsFloat for Complex).
> 
> wrote:
> I've fixed this already in a later patch. I would link you to the relevant entry on \
> anonsvn but it seems to be broken at the moment.

Okay, I was going by just the bits that are here on RB.


> On 2009-09-08 17:52:43, Matthew Woehlke wrote:
> > /trunk/KDE/kdelibs/kate/syntax/data/python.xml, line 383
> > <http://reviewboard.kde.org/r/1513/diff/2/?file=11087#file11087line383>
> > 
> > Hard-coded colors are evil; in syntax files especially so. Have you considered if \
> > there is a better way?
> 
> wrote:
> A search through the syntax highlighters shows over 400 instances of manual \
> suggestions of colours. I therefore assert that this is the standard thing to do \
> when adding a new language construct which Kate does not automatically highlight; \
> there is significant precedent for doing it this way at least. 

I can find lots of instances of hard-coded colors in applications (in general). For \
example, I recently fixed one in the vi-mode status bar, and a few in KAlarm (there \
remain others). And there are hard-coded colors in Thunderbird and QGit. That doesn't \
make it right to use hard-coded colors. That just makes it a /common/ bad practice.

While I will agree that it may be unavoidable to some extent at the moment ¹, it is \
best to make use of as many of kate's styles as possible before resorting to this, \
because it means you WILL have illegible text for some people that will have to go in \
and change it.

( ¹ That's why I didn't say "don't do this", I asked "[if] you considered if there is \
a better way". Your reply makes me think the answer to my question is "no".)

Please don't use "everyone else does it" as an excuse to perpetuate brokenness. \
(Again, I am not saying "you may not do this", just stop and think about the \
consequences and be sure you are convinced this is the lesser evil.)


- Matthew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1513/#review2269
-----------------------------------------------------------


On 2009-09-07 09:37:22, pag wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1513/
> -----------------------------------------------------------
> 
> (Updated 2009-09-07 09:37:22)
> 
> 
> Review request for Kate.
> 
> 
> Summary
> -------
> 
> Update the Python syntax highlighting file:
> * Fully support string modifiers. Support was very weak and badly implemented \
>                 before.
> * Highlighting the leading "u" on unicode strings as part of the string
> * Makes operators bold -- currently they're plain and easily ignored.
> 
> Am I OK to commit this?
> 
> 
> Diffs
> -----
> 
> /trunk/KDE/kdelibs/kate/syntax/data/python.xml 1020747 
> 
> Diff: http://reviewboard.kde.org/r/1513/diff
> 
> 
> Testing
> -------
> 
> Tested before and after against \
> http://paul.giannaros.org/stuff/test_python_syntax.py  
> 
> Screenshots
> -----------
> 
> String formatting before & after
> http://reviewboard.kde.org/r/1513/s/199/
> 
> 
> Thanks,
> 
> pag
> 
> 

_______________________________________________
KWrite-Devel mailing list
KWrite-Devel@kde.org
https://mail.kde.org/mailman/listinfo/kwrite-devel


[prev in list] [next in list] [prev in thread] [next in thread]