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 |
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 |
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 |
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 |
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 |
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 |
Free forum by Nabble | Edit this page |