Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology
Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 973 by [hidden email]: Roassal font organizer singleton  
inconsistencies
http://code.google.com/p/moose-technology/issues/detail?id=973


Following on from Issue 971, use of singletons related to the Roassal font  
organizer seems not used consistently.

Using Finder, I've searched all 'Source' of Roassal packages, for the  
following both 'ROFontOrganizer' and 'fontOrganizerClass' and found...

----Part1--------------

     ROAbstractLabel >> fontFor: anElement with: aCamera
        fo := ROPlatform current fontOrganizerClass.

     ROAbstractLabel >> offsetWhenDrawing
         ^ ROPlatform current fontOrganizerClass

     ROAbstractLabel >> widthOfLine: string withFont: aFont
         ^ ROFontOrganizer current widthOfString: string font: font.

I propose the last is changed to...

     ROAbstractLabel >> widthOfLine: string withFont: aFont
         ^ ROPlatform current fontOrganizerClass widthOfString: string font:  
font.

and also delete...

     ROFontOrganizer >> current
        ^ self subclasses first

which seems plain wrong.

----Part2--------------

Also I'm a perplexed by the way both of these methods

     ROMorphicPlatform>>fontOrganizerClass
        ^ ROFontOrganizerMorphic

     ROPharoAthensPlatform>>fontOrganizerClass
        ^ ROFontOrganizerAthens

override this one

     ROPlatform>>fontOrganizerClass
        ^ fontOrganizerClass

such that the instance-variable 'fontOrganizerClass' is ignored, as well as  
the following seeming irrelevant...

     ROPlatform>>fontOrganizerClass:
         ^ fontOrganizerClass := aClass.

Is ROPlatform ever instantiated, or abstract?
A search for all references to 'ROPlatform' in the 'Source' of all Roassal  
finds...

* ROPlatform current (x40)
* ROPlatform platforms (x2)
* ROPlatform removeNamed: (x1)
* ROPlatform add: (x2)
* ROPlatform setCurrent: (x1)
* ROPlatform new (x1)

The last is only in a test ROPlatformTest>>testAddingAPlatform.

I propose to modify...

     ROPlatform>>fontOrganizerClass
         self subclassResponsibility

delete....
     fontOrganizerClass instance-variable

delete...
     ROPlatform>>fontOrganizerClass:

from...
     ROPlatformTest>>testAddingAPlatform
remove line...
        platform fontOrganizerClass: ROFontOrganizer subclasses anyOne.


hope this helps,
cheers -ben


--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology

Comment #1 on issue 973 by [hidden email]: Roassal font organizer  
singleton inconsistencies
http://code.google.com/p/moose-technology/issues/detail?id=973

After making the changes proposed above, all tests are green except  
ROPlatformTest>>testAddingAPlatform. The problem is since ROPlatform is  
instantiated, then sent to #add: which calls #check which calls  
ROPlatform>>fontOrganizerClass which is now subclassResponsibilty.

So further propose adding...

     ROPlatform subclass: #ROPlatformSubclassTestResource

and

     ROPlatformSubclassTestResource >> fontOrganizerClass
        ^ ROPlatform subclasses anyOne new fontOrganizerClass.

and modifying ROPlatformTest>>testAddingAPlatform ...
        platform := ROPlatformSubclassTestResource new.

This makes all Roassal tests green.

--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology
Updates:
        Status: Started

Comment #2 on issue 973 by [hidden email]: Roassal font organizer  
singleton inconsistencies
http://code.google.com/p/moose-technology/issues/detail?id=973

Hi Ben,

Thanks for having a look at this. Indeed, the platform management is not  
the best.
I have introduced your first change:

     ROAbstractLabel >> widthOfLine: string withFont: aFont
         ^ ROPlatform current fontOrganizerClass widthOfString: string font:  
font.


Any idea how to remove:
ROFontOrganizer >> current
        ^ self subclasses first
?

For the fontOrganizerClass, I agree with you. But I feel what you propose a  
rather ad-hoc. We need to rethink completely the platform implementation I  
guess.


--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology

Comment #3 on issue 973 by [hidden email]: Roassal font organizer  
singleton inconsistencies
http://code.google.com/p/moose-technology/issues/detail?id=973

> Any idea how to remove:
> ROFontOrganizer >> current
> ^ self subclasses first

Not sure what you mean. Just delete it :)
Oh, you mean justification...

Use Finder > Choose Packages > "Roassal" only
then search for 'ROFontOrganizer' and you get only three references...
* ROPlatformTest>>testAddingAPlatform
* ROMorphicPlatform>>fontOrganizerClass
* ROPlatform>>check
...none of which send #current to ROFontOrganizer.



--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology

Comment #4 on issue 973 by [hidden email]: Roassal font organizer  
singleton inconsistencies
http://code.google.com/p/moose-technology/issues/detail?id=973

> For the fontOrganizerClass, I agree with you. But I feel what you propose  
> a rather ad-hoc. We need to rethink completely the platform  
> implementation I guess.

What is it that you don't like? ROPlatformSubclassTestResource ?

Its easier to pick holes than to fix them, but as a rough guess from the  
outside, ROPlatform seems like it was originally designed to be  
instance-based, storing different platform configurations as  
instance-objects, but was later implemented more class-based using  
polymorphic-constants.  I would never have considered the latter prior to  
using Smalltalk, but I'm tending to like the approach. It seems more  
intention revealing and visible to have it in code rather than hidden as  
data inside an instance-object.

So the big question is which approach is preferred. If the latter, maybe  
the same should be considered for the other instance-variables of  
ROPlatform (as seen in ROPlatform>check.)  Then maybe you don't need  
ROPlatform>>add:, hence not need ROPlatform>>check, hence not need my  
proposed ROPlatformSubclassTestResource.


The only alternatives I can see are:

1. Every instance-method of ROMorphicPlatform and ROPharoAthensPlatform  
returns a constant. So if you prefer the instance-based way, then both of  
these classes should be deleted.

2. I tried altering ROPlatformTest>>testAddingAPlatform to have...
platform := ROPlatform subclasses anyOne new.
but that fails the test differently, since the line...
platform name: 'test' is irrelevant, and #name returns a constant in the  
subclasses, and in ROPlatform>>add: it gets stored for instance in the  
dictionary at 'morphic' rather than 'test'.

3. If you go with the class-base polymorphic-constants, then delete  
ROPlatform_class>>add: and hence ROPlatformTest>>testAddingAPlatform (as  
well as ROPlatform>>check), and do the check as a test like...

ROPlatformTest>>testValidPlatforms
     ROPlatform subclasses do:
     [   :class |
         class fontOrganizerClass superclass == ROPlatform.
         "etc..."
     ]



--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev
Reply | Threaded
Open this post in threaded view
|

Re: Issue 973 in moose-technology: Roassal font organizer singleton inconsistencies

moose-technology
Updates:
        Status: WontFix

Comment #5 on issue 973 by [hidden email]: Roassal font organizer  
singleton inconsistencies
https://code.google.com/p/moose-technology/issues/detail?id=973

(No comment was entered for this change.)

--
You received this message because this project is configured to send all  
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings
_______________________________________________
Moose-dev mailing list
[hidden email]
https://www.iam.unibe.ch/mailman/listinfo/moose-dev