Prevent font chooser open error listing #638
Conversation
…ected aFont. If we do not find a font family matching aFont (for example not yet loaded fonts), inform and then select the first family in the system. If we do not find a font style, select the first one available as initial style
|
The failing test testUpdateExtensionSubscription is unrelated to the fix |
| @@ -347,13 +347,18 @@ StFontChooserPresenter >> updatePreview [ | |||
|
|
|||
| { #category : 'initialization' } | |||
| StFontChooserPresenter >> updateWithFont: aFont [ | |||
| "Try to update the receiver's listing with the selected aFont. | |||
| If we do not find a font family matching aFont (for example not yet loaded fonts), inform and then select the first family in the system. | |||
| If we do not find a font style, select the first one available as initial style" | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure I would select the first one.
Why not just abort the current update of font?
There was a problem hiding this comment.
Christoph could you reproduce the bug in your image? (note that it requires you to select a non default font). It is not about canceling to open the lookup but to prevent an error when you have already a configured font.
There was a problem hiding this comment.
I just reproduced the problem.
I do not know where this presenter is used but If I understand correctly, this presenter can only works if a font is selected, right?
In this case, the fix is ok.
| self shouldnt: [ | ||
| | window | | ||
| window := presenter open. | ||
| window close ] raise: Error |
There was a problem hiding this comment.
The goal of a test is to capture an error if any.
Then shouldnt: raise: Error is useless but the test is also useless.
The method should just be named openPresenter.
There was a problem hiding this comment.
I don't understand. The presenter configured in the setUp fails to open if the fix in the PR is not applied. As stated in the issue:
| fontChooser presenter |
fontChooser := FontChooser newWithAllFamilies
title: 'Title';
font: (LogicalFont
familyName: 'Arial'
pointSize: 11
stretchValue: 5
weightValue: 300
slantValue: 0);
onAcceptDo: [ : e | self halt ].
presenter := StFontChooserPresenter on: fontChooser.
presenter open.See the following screenshot:
The method is named openPresenterShouldntRaiseError since it is common code called from multiple tests:
StFontChooserPresenterTest>>#testOpenAllOnNonExistantFontFamily StFontChooserPresenterTest>>#testOpenDefaultOnUnloadedFontFamily StFontChooserPresenterTest>>#testOpenDefaultOnNonExistantFontFamily StFontChooserPresenterTest>>#testOpenAllOnUnloadedFontFamily
There was a problem hiding this comment.
What I mean is that a method test should never include:
self shouldnt: [ ] raise: ErrorIt is the purpose of the test framework to capture all Errors and to make your test fail if any.
It would only make sense if you wanted to check for a specific error that should not happen.
You should rename StFontChooserPresenterTest>>#openPresenterShouldntRaiseError to StFontChooserPresenterTest>>#openPresenter with:
openPresenter
| window |
window := presenter open.
window close 
Issue
Opening font chooser with a non-existent font family raises an exception.
Solution
Try updating Font Chooser with the selected font; if not found, inform and select the first available font family and style.
Add tests
Fixes #637.