Community
Participate
Working Groups
The TextSizeDetermination is constituted of at least 7 classes which all are in org.eclipse.swt.internal.graphics. If moving some currently nested classes intofiles of their own, the number even gets larger. Therefore I suggest to move them to a package of their own, e.g. org.eclipse.rwt.internal.textsize What do you think?
I consider this a good idea but it should be done not until a final solution for #341714 is available. I'm currently working on this and it could cause a lot of conflicts :-) I also propose to change the names of some of the classes and methods involved to get from a logical point of a more consistant subsystem. 1) TextSizeDetermination has three public methods called stringExtent, textExtent and markupExtent. So we got the word text in TextSizeDetermination but string-,text and markupExtent methods which looks like a mismatch to me. We also use size and extent as synonyms here. To me it seems more stringent to have a class called ExtentDetermination: Point ExtentDetermination.stringExtent( Font, String ) Point ExtentDetermination.textExtent( Font, String, int ) Point ExtentDetermination.markupExtent( Font, String, int ) Unfortunately this leads to a duplication of the word 'extent' in calling statements. So how about Point ExtentDetermination.string( Font, String ) Point ExtentDetermination.text( Font, String, int ) Point ExtentDetermination.markup( Font, String, int ) I also played around with Point ExtentDetermination.ofString( Font, String ) Point ExtentDetermination.ofText( Font, String, int ) Point ExtentDetermination.ofMarkup( Font, String, int ) which looks a little bit uncommon but is the version which I consider the best readable one... 2) The XXXextent methods of TextSizeDetermination use the method doMeasurement for the actual workload. It does a lookup in the database, if nothing found it calls the estimation and afterwards hands over the string to measure to the calculation mechanism. Doing some refactorings for #341714 I found it more plausible to actually have methods for lookup, an estimatation and a measurement handling. I say measurement handling instead of calcuation since IMHO this is what happens in the latter case. The names like ICalculationItem seems wrong to me since there's no calculation involved here. The classes and methods involved are doing a measurement of the extent of a certain text, string or markup on the client side. So we should call those classes and methods accordingly: IMeasurementItem, MeasurementItemImpl, addMeasurementItem etc. I even would prefer a ExtentMeasurementHandler instead of TextSizeDeterminationHandler. 3) The remaining classes should be renamed of course in the same manner (ExtentEstimation etc.). I think we could do this without any problems since as far as I see there is no API breakage involved. While working on #341714 I could create a patch that provides a first shot of such refactorings and also move all the classes into an own package. How about extentdetermination?
After discussing this issue with Rüdiger we came up with the following idea: 1) move all classes in their own package called org.eclipse.rwt.internal.textsize 2) rename TextSizeDetermination into TextSize 3) rename XXXExtent into forXXX: TextSize.forString(...) TextSize.forText(...) TextSize.forMarkup(...) 4) change the internal mechansim to use lookup, estimate and measure as meaningful verbs for method names. Also rename classes like ICalculationItem to IMeasurementItem and TextSizeDeterminationHandler to MeasurementHandler for example. 5) Move measurement related methods and fields to MeasurementHandler or appropriate utility class.
+1 for an extra package Though I like short names, I think that "TextSize" suggests to be an instantiable type. A static method like "forString()" looks like a factory method, and I would expect the result of TextSize.forString(...) to be of type TextSize: TextSize size = TextSize.forString( string ); Since TSD is a util class, I'd suggest to rename it to TextSizeUtil. We've have used this pattern all over and I think we should retain it now. Regarding the methods stringExtent() and textExtent(), these methods provide the implementation of Graphics#stringExtent() and textExtent(), which are mostly used as replacements for GC#stringExtent() and textExtent(). These methods are designed to produce similar results as their GC counterparts wrt to newline (and tab?) handling. Therefore, I think using the same names makes at least some sense. Without this background, the difference between forString() and forText() does not become clear. If we change those names, shouldn't we find names that outline the difference?
After discussing this issue a little bit more with Ralf and Rüdiger I must agree that changing the method names of stringExtent and textExtent would cause some kind of inconsistency regarding the API methods GC#XXXextent and Graphics#XXXextent. So the final solution covers the following: 1) move the text size handling classes into an own package called org.eclipse.rwt.internal.textsize 2) rename TextSizeDetermination to TextSizeUtil 3) change the internal mechansim to use lookup, estimate and measure as meaningful verbs for method names. Also rename classes like ICalculationItem to IMeasurementItem and TextSizeDeterminationHandler to MeasurementHandler for example. 4) Move measurement related methods and fields to MeasurementHandler or appropriate utility class.
There is one last thing that we disscussed but that I missed in may last comment: To complete this refactoring we should ensure that all widgets use the Graphics or GC API methods for text size determination. Some widgets currently use the internal TextSizeDetermination class directly.
(In reply to comment #5) > There is one last thing that we disscussed Actually there is another thing... To the best of our knowledge there is no use of TextSizeDetermination#markupExtent - therefore it should be removed.
The changes mentioned in comment 4, 5 and 6 are in CVS head. The textsize determination mechanism has been thoroughly refactored and the test coverage has been improved. Coupling with other subsystems (e.g. EntryPointManager)has been reduced.
