[Konsole-devel] KDE/kdebase/apps/konsole/src

[Diego Iastrubni]

Hi Robert,

Sorry, I probably miss understood your previous message in the mailing list, 
by bad. I am replying to konsole-devel since I do want people to see and 
comment about it. I am also sending it to you directly, since I already lost 
a few mails on konsole-devel... better safe then sorry.

See comments inline:
> > <string>Enable BiDi rendering </string>
>
> Assuming there is enough horizontal room, could you use "Bi-directional
> text rendering" rather than "BiDi rendering" here.
Ok, it's also the text in konsole3. I will fix this.

> > (valid for Arabic, Farsi or Hebrew only)
> By that I assume you mean that this setting has no effect in other
> scripts?
If I understand correctly how the BiDi algorithm works, and how it's 
implemented in scribe, that is a correct assumption. This should only affect 
the re-arrangement of "RTL" characters, it's it's not - it's a but in the Qt 
scribe implementation, or a corner case (and I don't have an example) which 
can be avoided by "enabling" the BiDi rendering - which just passes the 
original string to QPainter::drawText(). 

I honestly don't see a problem here.

> > In QPainter3, there was an option to draw a text and tell the painter
> > "please do not implement BiDi". In QPainter4 (up to 4.4) there
> > is no such option. I am forcing it by drawing
> > "painter.drawText(rect,0,QChar(0x202D)+text)"
> > (that is &LRO;), and it does seem to work,
>
> If you need to use a non-obvious method to do something please add
> comments to the code to explain what you are doing and how it works.
> This is especially true if it is a workaround for an omission in Qt, KDE
> or some other library which might be fixed in the future.
>
> In this case replacing the literal "0x202D" with a suitably named
> constant (eg. "LTR_OVERRIDE_CHAR") would also help.
Ok, I will document this part of code a little more. You are right.

> Where there is a lengthy discussion leading up to the change, a link to
> the mailing list thread may help.
again, I will link to that thread. In the code and in the commit log.

> One small thing missing is that when adding a new property, you need to
> explicitly set a default value for it by setting the property value in
> the FallbackProfile class constructor in Profile.cpp.  Profiles that
> don't customise this setting will inherit this default value from
> FallbackProfile.
Funny how I missed that in my testings :)

> > quickly enable/disable it (alt+control+b in konsole3),
> > but I am not sure what is the best way for doing it.
>
> The simplest method would be to add a "Bi-directional text" toggle
> action to the View menu, just below the "Set Character Encoding" action.
Are you sure you want to pollute the menus with this option? It will not be 
used that much and IMHO it will just waste space. I would like to do this 
similar to konsole3, where I could set the shortcut without a GUI element. 
This will still be visible in the tooltip and in the shortcut dialog. Well, 
at least this is the implementation I would like to see.

I am not sure if I will have time to fix this today, and the working week 
starts in Sunday here (crazy country...), so worst case scenario I will be 
able to fix these issues next week. The text change should be done ASAP, 
before scripty starts it's work, otherwise about 60 teams will get to 
translate that twice. But this will still leave this patch half working, is 
this ok by you? or would you like to revert it and have a better patch by 
next week?