Fixed settings on Pharo 7 (#1) #12
Conversation
fortizpenaloza
changed the title
|
It works on Pharo 6 also. |
| @@ -26,8 +26,8 @@ MICarousel class >> activate: aBoolean [ | |||
|
|
|||
| { #category : #'initialize-release' } | |||
| MICarousel class >> initialize [ | |||
| super initialize. | |||
There was a problem hiding this comment.
Why removing the call to super here?
There was a problem hiding this comment.
While reading your code, I saw a code critic rule discouraging sending #initialize to super on the class-side since some class of the top of the hierarchy should not be reinitialized.
I also browsed the Morph hierarchy and I didn't find a single super initialize collaboration on the class initialization.
There was a problem hiding this comment.
Ah yeah, you're right I didn't realised it was on class side.
| } | ||
|
|
||
| { #category : #accessing } | ||
| MICarouselSettings class >> defaultThumbnailUpdateTime [ | ||
| ^ 5 seconds |
There was a problem hiding this comment.
Why did you removed the instantiation of a duration from here?
There was a problem hiding this comment.
I stumbled with this bug opening the settings after loading the dev branch on Pharo 7.
A debugger opens while building a PluggableSliderMorph. It fails scaling the default value (a 5 seconds duration) within an integer range from 1 to 30 configured on MICarouselSettings class>>#wpThumbnailUpdateTimeSettingsOn:.
The Duration>#roundTo: expects another duration. I spent a while trying to fix it keeping the duration and, after failing miserably, I finally changed it for an integer.
| @@ -39,7 +39,7 @@ SystemWindow >> mirageThumbnailPropertySymbol [ | |||
| { #category : #'*Mirage-Carousel' } | |||
| SystemWindow >> scheduleThumbnailUpdate [ | |||
| ([ | |||
| MICarouselSettings thumbnailUpdateTime wait. | |||
| MICarouselSettings thumbnailUpdateTime seconds wait. | |||
There was a problem hiding this comment.
This is a consequence of what I describe above.
|
You did a great job btw, I totally love this project. I missed window navigation so much. |
|
Thanks for your contribution. I merge this PR. |
Integrate fix for issue #12
Fixes #10