Merge lp://qastaging/~laurent-dufrechou/ipython/ipython-sync-frontend-retweaks into lp://qastaging/~gael-varoquaux/ipython/ipython-sync-frontend

- Corrected a behavior of 'enter' that was repeatedly sent. Now it wait key up event.
- Moved most of needed constant inside the console class.
- Moved prompt into wx_controller class
- Added "--style black" option to ipyhonx to show how all works

Note: we need to check the need for ANSI_COLORS. Seems not usefull at all if we use the scintilla lexer.

Way to improve:
- option to enable/disable anti-aliasing?
- option to enble/disable scintilla lexer (so user can have line side by side and no color glich on first key press)

- Color of current editing line has not the same behaviour under windows. Perhpas you should check this gael.

On Sun, Feb 22, 2009 at 10:56:47PM -0000, Laurent Dufrechou wrote:
> - Corrected a behavior of 'enter' that was repeatedly sent. Now it wait key up event.
> - Moved most of needed constant inside the console class.
> - Moved prompt into wx_controller class
> - Added "--style black" option to ipyhonx to show how all works

OK, I am reviewing this right now. Will send feedback as I go through.

> Note: we need to check the need for ANSI_COLORS. Seems not usefull at
> all if we use the scintilla lexer.

They are useful when the output of a command is captured.

> Way to improve:
> - option to enable/disable anti-aliasing?
> - option to enble/disable scintilla lexer (so user can have line side by side and no color glich on first key press)

If you want to add these as boolean attributes on the class, I am OK.

> - Color of current editing line has not the same behaviour under windows. Perhpas you should check this gael.

Not sure what you mean here, but I'll check.

First remarks (there will be other's):

console_widget, line 124: please don't put logics in a class definition.
Either put this logic at the module level, or in the class
initialisation. In addition, please move all the attribute definitions at
the top of the class, unless you are separating them by interface they
provide.

Will go on reading and providing feedback.

Gaël

Ho by the way, I found scintilla lexer not very good.
example:
for i in range(len("toto')):

you will see that it will colourize 'for' and 'in' not range and len :(
Morover there is the bug with the stc.Colourize. You can tell
scintilla to colourize from char 50 to char 100, it will always
colourize previous line.
So have you thought about using pygments?
http://pygments.org/

Perhaps it will give us more flexibility, instead having to workaround
scintilla...

>> Note: we need to check the need for ANSI_COLORS. Seems not usefull at
>> all if we use the scintilla lexer.
>
> They are useful when the output of a command is captured.
>

Hum ok, but the way you where using it, I mean calling
self._apply_styles then styleClearAll disabled their effect. Or did I
miss something?
I tried to activate them (either putting self._apply_styles at end of
_configure_scintilla or just after styleClearAll) but it interferered
a lot with the display :(.

>> Way to improve:
>> - option to enable/disable anti-aliasing?
>> - option to enble/disable scintilla lexer (so user can have line side by side and no color glich on first key press)
>
> If you want to add these as boolean attributes on the class, I am OK.
>
>> - Color of current editing line has not the same behaviour under windows. Perhpas you should check this gael.
>
> Not sure what you mean here, but I'll check.
I mean, launch ipythonx -s black, then on current input try:
def hello(a): #will colourize in grey the background
  print "cool" #hey under linux ok (grey) under windows it is updated
only after pushing enter :( (so black while editing and grey after)

>
> First remarks (there will be other's):
>
> console_widget, line 124: please don't put logics in a class definition.
> Either put this logic at the module level, or in the class
> initialisation. In addition, please move all the attribute definitions at
> the top of the class, unless you are separating them by interface they
> provide.
>

Ok will check after getting all your remarks.

On Mon, Feb 23, 2009 at 12:06:49AM +0100, Gael Varoquaux wrote:
> Will go on reading and providing feedback.

console_widget:

What is the point of having a 'set_new_style' method that only calls the
'_configure_scintilla' method. If there is a good reason to rename the
later, lets rename it. But not introduce multiple confusing methods.

By the way, if you remove a line, please don't just comment it, as you
did on line 292 of console_widget. Go ahead and delete it. That's why we
have version control :).

On the positive side, I like what you did merging _configure_scintilla
and _apply_style, and the clean up of this interface with a dictionnary.

line 435: why the parenthesis?

ipythonx:

I don't like the huge dictionnary of options hard coded in the
initialiser. It smells just as bad as what I had done previously. Why
don't you make the class accept an options dictionnary in the
constructor, and pass this dictionnary to the class, rather than a string
('black'). This will make it more versatile. You can add the prompt_in1
and prompt_in2 in this dictionnary. To get the behavior you implemented
for the ipythonx application, just define the 'black' dictionnary at the
module level, and use the commandline switch to pass it to the instance
creation. That way it is much easier for someone reusing this code to
create a new style: just copy the module-level dictionnary and pass it to
the class on instantiation. Actually, I believe this code shoudl all go
into console_widget, along-side with the default 'white' style
dictionnary (which should also be a module-level object). This is all
related to the widget. You can import the black style from console_widget
in ipythonx to deal with command line arguments.

wx_frontend:

I am OK with all changes.

I know I am a bit fussy, but if we keep these few modules as clean and as
versatile as possible, they can serve as the basis of various GUIs for a
long time, so I am putting quality quite high.

Thanks for your time.

Gaël

Done.
Can you check the need for:

    'stdout' : 'fore:#0000FF',
    'stderr' : 'fore:#007f00',
    'trace' : 'fore:#FF0000',
?

I'm not sure they are usefull. Same as ANSI_STYLES.
ANSI_STYLES is only usefull when lexer is disabled. (on your code it was in fact innecfective (I think)).

On Mon, Feb 23, 2009 at 10:59:40PM -0000, Laurent Dufrechou wrote:
> Done.
> Can you check the need for:

> 'stdout' : 'fore:#0000FF',
> 'stderr' : 'fore:#007f00',
> 'trace' : 'fore:#FF0000',
> ?

Correct, this is not useful, you can kill it.

> I'm not sure they are usefull. Same as ANSI_STYLES. ANSI_STYLES is only
> usefull when lexer is disabled. (on your code it was in fact
> innecfective (I think)).

Hum. Darn. They used to work. I'll have to look at that (and no I don't
believe that this can only work when the lexer is disabled).

Thanks for your feedback,

Gaël

Hey guys, what's the status on this one? I also noted that this is a branch on top of Gael's, but Gael's isn't currently marked for review/merge. We should basically sort out the two steps, if possible for 0.10:

1. Get Laurent's done on top of Gael's
2. Get Gael's ready for merge into trunk.

Is that your (collective you) plan?

f

On Sun, Mar 15, 2009 at 11:01:41PM -0000, Fernando Perez wrote:
> Hey guys, what's the status on this one? I also noted that this is a branch on top of Gael's, but Gael's isn't currently marked for review/merge. We should basically sort out the two steps, if possible for 0.10:

> 1. Get Laurent's done on top of Gael's
> 2. Get Gael's ready for merge into trunk.

> Is that your (collective you) plan?

We just discussed this today :). Plan is: Laurent does a few
modifications I asked, and we merge during the coming week.

Gaël

Hey, i took a look of what I did and in fact all things we discussed was corrected long time ago :)

So think we could merge.

On Tue, Mar 17, 2009 at 08:02:52PM -0000, Laurent Dufrechou wrote:
> Hey, i took a look of what I did and in fact all things we discussed was corrected long time ago :)

> So think we could merge.

OK, I'll look at it tonight or tomorrow night. Thanks a lot.

Gaël