13204 Hard to see Morph halo label

Build 30507 integrated slice 11492 "Kill UpdatingStringMorph, UpdatingMenu, and NameInHaloMorph"
that removed class UpdatingStringMorph its subclass NameInHaloMorph.  As a consequence the halo "morph label"
became hard to read.  Compare the red circled areas in the snapshots from build.



Technically its pretty easy fix. Restoring just the #drawOn: part of NameStringInHalo (now subclassed from StringMorph) solves the problem as follows...
Adding...
    StringMorph subclass: #NameStringInHalo
        instanceVariableNames: ''
        classVariableNames: ''
        category: 'Morphic-Base-Widgets'

    NameStringInHalo>>drawOn: aCanvas
        aCanvas fillRectangle: self bounds color: Color white.
        super drawOn: aCanvas.

and modify the following line in HaloMorph>>addNameBeneath:string: ...
    nameMorph := StringMorph contents: aString font: StandardFonts haloFont.
to..
    nameMorph := NameStringInHalo contents: aString font: StandardFonts haloFont.

However I'm seeking a broader picture to understand if that is the right way to proceed...
1. Was NameStringInHalo particularly undesired or was it more a consequence of UpdatingStringMorph being removed ?

2. Should the same class name "NameStringInHalo" be used having a different superclass than before. Actually as I write that I am thinking this creates an unnecessary clash with Squeak and derivatives that still have NameStringInHalo, and so...

3. Any suggestions for another more generic class name?  SolidStringMorph, OpaqueStringMorph, etc

4. Any suggestions for another way for a StringMorph to have a solid background?

cheers -ben

why do we need a special class for that?
Why not just a StringMorph with correct background?

Build 30507 integrated slice 11492 "Kill UpdatingStringMorph, UpdatingMenu, and NameInHaloMorph"
that removed class UpdatingStringMorph its subclass NameInHaloMorph.  As a consequence the halo "morph label"
became hard to read.  Compare the red circled areas in the snapshots from build.



Technically its pretty easy fix. Restoring just the #drawOn: part of NameStringInHalo (now subclassed from StringMorph) solves the problem as follows...
Adding...
    StringMorph subclass: #NameStringInHalo
        instanceVariableNames: ''
        classVariableNames: ''
        category: 'Morphic-Base-Widgets'

    NameStringInHalo>>drawOn: aCanvas
        aCanvas fillRectangle: self bounds color: Color white.
        super drawOn: aCanvas.

and modify the following line in HaloMorph>>addNameBeneath:string: ...
    nameMorph := StringMorph contents: aString font: StandardFonts haloFont.
to..
    nameMorph := NameStringInHalo contents: aString font: StandardFonts haloFont.

However I'm seeking a broader picture to understand if that is the right way to proceed...
1. Was NameStringInHalo particularly undesired or was it more a consequence of UpdatingStringMorph being removed ?

2. Should the same class name "NameStringInHalo" be used having a different superclass than before. Actually as I write that I am thinking this creates an unnecessary clash with Squeak and derivatives that still have NameStringInHalo, and so...

3. Any suggestions for another more generic class name?  SolidStringMorph, OpaqueStringMorph, etc

4. Any suggestions for another way for a StringMorph to have a solid background?

cheers -ben

[hidden email] wrote:

why do we need a special class for that?
Why not just a StringMorph with correct background?

Because I don't know how to do that.   I describe the simplest fix I know how, showing that it is simple,  and seek further advice from the list. See question 4 below.
cheers -ben

On 17/4/14 16:20, Ben Coman wrote:

Build 30507 integrated slice 11492 "Kill UpdatingStringMorph, UpdatingMenu, and NameInHaloMorph"
that removed class UpdatingStringMorph its subclass NameInHaloMorph.  As a consequence the halo "morph label"
became hard to read.  Compare the red circled areas in the snapshots from build.



Technically its pretty easy fix. Restoring just the #drawOn: part of NameStringInHalo (now subclassed from StringMorph) solves the problem as follows...
Adding...
    StringMorph subclass: #NameStringInHalo
        instanceVariableNames: ''
        classVariableNames: ''
        category: 'Morphic-Base-Widgets'

    NameStringInHalo>>drawOn: aCanvas
        aCanvas fillRectangle: self bounds color: Color white.
        super drawOn: aCanvas.

and modify the following line in HaloMorph>>addNameBeneath:string: ...
    nameMorph := StringMorph contents: aString font: StandardFonts haloFont.
to..
    nameMorph := NameStringInHalo contents: aString font: StandardFonts haloFont.

However I'm seeking a broader picture to understand if that is the right way to proceed...
1. Was NameStringInHalo particularly undesired or was it more a consequence of UpdatingStringMorph being removed ?

2. Should the same class name "NameStringInHalo" be used having a different superclass than before. Actually as I write that I am thinking this creates an unnecessary clash with Squeak and derivatives that still have NameStringInHalo, and so...

3. Any suggestions for another more generic class name?  SolidStringMorph, OpaqueStringMorph, etc

4. Any suggestions for another way for a StringMorph to have a solid background?

cheers -ben

pharo4Stef@free.fr wrote

Why not just a StringMorph with correct background?

That doesn't look possible with the current implementation. It just draws the string with no background:
  StringMorph>>#drawOn: aCanvas
        ...
        "super drawOn: aCanvas. <- This line doesn't exist"
        aCanvas drawString: contents in: bnd font: self fontToUse color: color.

From the "... color: color" above, it looks like StringMorph is hijacking the instVar it inherited from Morph, which usually means background color, to use for the text color.

If you hack drawOn: to
        ...
        self color: Color white.
        super drawOn: aCanvas.
        aCanvas drawString: contents in: bnd font: self fontToUse color: Color black.
you get the halo label you're looking for but of course break everything else.

So I think that if we want StringMorphs to have backgrounds, we have to add a textColor instVar, initialize color to Color transparent, and rewrite all color assignments in StringMorph to set textColor.

In short: not for 3.0.

Sean P. DeNigris wrote:

[hidden email] wrote
  

Why not just a StringMorph with correct background?
    

That doesn't look possible with the current implementation. It just draws
the string with no background:
  StringMorph>>#drawOn: aCanvas
	...
	"super drawOn: aCanvas. <- This line doesn't exist"
	aCanvas drawString: contents in: bnd font: self fontToUse color: color.

>From the "... color: color" above, it looks like StringMorph is hijacking
the instVar it inherited from Morph, which usually means background color,
to use for the text color.

If you hack drawOn: to
	...
	self color: Color white.
	super drawOn: aCanvas.
	aCanvas drawString: contents in: bnd font: self fontToUse color: Color
black.
you get the halo label you're looking for but of course break everything
else.

So I think that if we want StringMorphs to have backgrounds, we have to add
a textColor instVar, initialize color to Color transparent, and rewrite all
color assignments in StringMorph to set textColor.

In short: not for 3.0.
  

I agree that such deep changes can't be done at this late stage.  However the fix I described is low impact.  Is that reasonable to do as a temporary measure for 3.0 release, and a case opened to clean up in 4.0?  The class name could even be TemporaryOpaqueStringMorph with appropriate class comment.  I know I've left it late, but its annoyed me for months while I was working on other priority cases in the tracker.
cheers -ben

Ben Coman wrote

However the fix I described is low impact.  Is that reasonable to do as
a temporary measure for 3.0 release, and a case opened to clean up in
4.0?

That sounds reasonable to me. It seems simple and isolated enough not to cause any surprises...

OK I understand.

Stef

On 18/4/14 01:12, Sean P. DeNigris wrote: