Issue 4609 in pharo: Syntax highlighting broken

Status: Accepted
Owner: ----
CC: [hidden email]
Labels: Milestone-1.3

New issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Does anybody has a version of Shout that correctly works in the
traditional tools in Pharo 1.3?

I tried various versions of Shout, but could not find one that worked
correctly in the traditional tools. The problem I see is that Shout is
not properly setup/updated anymore, i.e. it does not callback to the
model (#shoutAboutToStyle:). When the browser/debugger changes the
method/definition/comment highlighting is not enabled/disabled and the
context is not changed.

Does anybody have a working version of Shout and/or knows how to
easily make it work?

It works in OB because I subclassed 3 editor classes and undone the
"new" way of how highlighting works. I thought I bring this up again,
because as far as I see there is still the same problem ...



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-1.4

Comment #1 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #2 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Also note that for the reported reason the old browser class definitions  
are not highlighted anymore, and the comment pane is accidentally  
highlighted.

Right now the behavior formerly in #shoutAboutToStyle: (the old correct  
code is still there, but not called), is distributed throughout the model  
code (some important parts are missing though). This makes it not only it  
impossible to replace Shout with anything else (good-bye Helvetia), but  
also starts to mix the model and the view: Why on earth would a model class  
(StringHolder) refer to (via the inst-var textMorph) a view class (one of  
the text morphs)?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #3 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

Lukas I would like to have a look at it. Where should I start looking?



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: [hidden email]

Comment #4 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

Lukas I reread and StringHolder is not really a model because it creates  
also the UI.

are you talking about

beStyled

        self textMorph ifNotNil: [:it | it beStyled].


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #5 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

Is it what we did with the NullStyler that is a problem?
We added the NullStyler so that we do not have explicit shout reference and  
that a text was styled byt a styler even a NullOne.




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #6 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

'StringHolder' and all its sub-classes are model classes. Also note that  
the super-class of 'StringHolder' is called 'Model'. Creating a UI doesn't  
make a class a view. Some classes (i.e. 'Debugger', 'Browser', ...) create  
different UIs with the same model class. It used to be possible to create  
non-morphic GUIs too.

There was a well designed dependency of the morphs (and possibly other  
widgets) and their model. The widgets are created by the model and depend  
on it. Multiple widgets (from different UI frameworks) can depend on the  
same model (this is broken now). Have a look at the code, there is not a  
single instance-variable referring to morphic in the whole hierarchy  
but 'textMorph'. This alone must ring a bell.

The NullStyler is not the problem. The problem is that the model decides  
how things are presented. The problem is that the model knows way too much  
about its view. The model literally hard-codes its (single) view and  
litters its code with (hard-coded) syntax highlighting. The styler has no  
possibility to get updated if the model doesn't decide to do so. The styler  
can only hope for the right information at the right time. No chance to  
change anything without patching. Even if the styler looks like it is  
pluggable, nothing else but Shout can ever be plugged-in.

I would vote to roll-back the editor changes in Pharo 1.3. This is my worst  
nightmare!


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #7 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

Guys, we need to do something about this.
Can we roll back the changes until we have a working version of them?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

I opened Hudson, downloaded a fresh 1.3 core image. Opened the shout repo,  
loaded the last Shout package.

Opened a Workspace, and I had colors. Opened the default Browser, I got  
colors too. Look for implementors, and also got colors.

So if the problem is only OB, maybe it's not a so big deal.


Do you have step to reproduce your "problem" ?


About StringHolder, we are removing it, so if some mechanisms are broken,  
it's not really important since it will removed asap.

And about NullStyler, I think that even if nothing else than Shout is  
pluggable for now, there is nothing else to be plugged. So maybe we  
shouldn't roll back, but improve the system instead


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Did you read the bug report at all? I did not say that there are no colors,  
i said that coloring is nor correctly enabled/disabled, that coloring is  
missing in parts, that coloring is wrongly enabled in parts, that coloring  
is hardcoded everywhere, that there is unused code (#shoutAboutToStyle:),  
that there are major design flaws (model that knows about its single view),  
that extensibility is impossible, ... OB works perfectly becaus I undone  
all Pharo 1.3 editor changes.

Now if you don't want to build an closed system this is ok for me. I can go  
back to Pharo 1.2. I am just saying that this bullet surgery is not healty.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 4609 by [hidden email]: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

could you precise which are the ob classes that you changed in order to  
make syntax highlighting work ? thanks


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #11 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Have a look at the package OB-Shout. The whole point of a *PluggableMorph  
class is to delegate all logic to the model (i.e. by calling back to  
#shoutAboutToStyle:). The current model forces one to keep a reference from  
the model to the view and tell the view what to do.

There is no reason why OB overrides  
OBPluggableShoutTextMorph>>#hasUnacceptedEdits:, #setText: and  
#useDefaultStyler other than to get into the flow and cause some  
side-effects at the right points in time that can reconfigure the  
highlighting.

Have a look at Shout in Pharo 1.2, it was a perfectly extensible and  
configurable system without any bad dependencies. The only culprit was that  
the default TextMorph couldn't be styled, but a special one was needed. But  
then this is probably the best design.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: -[hidden email]

Comment #12 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #13 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

Lukas for me a model should not depend on UI and from that perspective the  
complete stringHolder hierarchy is dependent (make references to widgets)  
so to me this is not because it inherits from a model that this is one.  
This hierarchy is broken since year.

I think that rollbacking is not an option. Now proposing solutions is a  
solution. Now I do not understand why you are all excited by StringHolder  
because we can to remove all the hierarchy from the system.




_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #14 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Stef, I am not excited about the StringHolder hierarchy. The problem is  
that the traditional tools (most notably the debugger) depend on it. I am  
just pointing out that with the changes to StringHolder and TextEditor in  
Pharo 1.3 the situation got way worse.

The implementation of StringHolder was never ideal, but it worked well. Now  
it is broken along with anybody depending on it. I am a bit surprised that  
you do not understand that the instance-variable 'textMorph' breaks the  
design of StringHolder. Also I am surprised that Pharo suddenly hard-codes  
the use of Shout everywhere; this makes experiments like Helvetia  
impossible.

Note that StringHolder never depended on widgets (or on the syntax  
highlighter) before Pharo 1.3. The factory methods in these classes (#open,  
#buildOn:, ...) could in fact be packaged separately with the view. These  
method only introduce a dependency from the views (Morphic, and earlier on  
MVC and ToolBuilder) to the model (as it should be). Could you please  
discuss this with Bert Freudenberg, I am sure he can explain this much  
better than I do.

I am at the end of my energy.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #15 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

We will fix it but can you understand that
    - we can do mistakes
    - it is difficult to understand what is the problem?

For example, I do not know if this is the integration of TextEditor or the  
changes to integrate shout that broke.
I still have to understand what broke and how we can fix it.

So statement like "Did you read the bug report at all?" does not help. Of  
course people read the problem, still they do not understand (probably too  
silly). So try to understand that mail communication is bad and it does not  
help. Take the perspective that people are nice and ready to help and fix  
things and that if they reply on the side this is just that they did not  
understand.





_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #16 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

So in 1.2.2

Model subclass: #StringHolder
        instanceVariableNames: 'contents textMorph'
        classVariableNames: ''
        poolDictionaries: ''
        category: 'Tools-Base'


in 1.3/1.4

Model subclass: #StringHolder
        instanceVariableNames: 'contents textMorph'
        classVariableNames: ''
        poolDictionaries: ''
        category: 'Tools-Base'


So I do not understand the problem. Again I feel bad because I look like an  
idiot.



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #17 on issue 4609 by [hidden email]: Syntax highlighting  
broken
http://code.google.com/p/pharo/issues/detail?id=4609

in #10517

Model subclass: #StringHolder
        instanceVariableNames: 'contents'
        classVariableNames: ''
        poolDictionaries: ''
        category: 'ST80-Kernel-Remnants'


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #18 on issue 4609 by renggli: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

Hi Alain,

I don't know why textMorph was added in the first place? From comment 16  
and 17 it seems that it is older. Now it is clearly a design mistake to  
have a view reference there, but if StringHolder is going to disappear  
anyway I don't care.

I suspected the variable textMorph was there because of the styling. The  
variable currently seems to be mostly used for that. The model should not  
be forced to know about the view and the styler. Instead, whenever text is  
updated, the selected styler should be notified so that it can call-back  
into the model to let it configure the styling. Shout would call  
#shoutAboutToStyle:, which is currently not sent from anywhere.

Lukas

On 25 August 2011 14:29, Alain Plantec <[hidden email]> wrote:
> Hi Lukas,
> So, AFAIK, the textMorph inst was badly added to StringHolder
> with a messy dependency from the model to an hardcoded view.
> So we should remove it and make shout work without it.
> Do I understand well ?
> Cheers
> Alain



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #19 on issue 4609 by [hidden email]: Syntax highlighting broken
http://code.google.com/p/pharo/issues/detail?id=4609

I try to understand. Here is a first step. OB tools seem ok to me with this  
cs.
tell me.

note that old browser/shout is broken with this cs. again, I try to  
understand and I will fix that later.

PluggableTextMorph has 5 instance variables which seems to be for styling:
I understand why we have styler and styled but why do we have also  
unstyledAcceptText, nullStyler and shoutedStyler inst vars ?

also, what is OBPluggableShoutTextMorph for ?
Normally this class should not exist, right ?
Shout should call #shoutAboutToStyle: whenever a text is modified, ok but  
from where is should be called ?







Attachments:
        Toward-OB-Shout-Fix.1.cs  1.4 KB


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker