Quick suggestion on the formatting. This one:

^ self canBrowseTopic or: [
  parentTopic ifNotNil: #canBrowseSubtopic ifNil: [false]]

could become:

^ self canBrowseTopic or: [
  parentTopic
ifNil: [false]
ifNotNil: [:topic | topic canBrowseSubtopic]]

or:

^ self canBrowseTopic
      or: [parentTopic notNil and: [parentTopic canBrowseSubtopic]]

Hmmm... what are other opinions on this? There is no need for #ifNil/ifNotNil in such a boolean expression?

Best,
Marcel


Am 13.10.2019 21:04:19 schrieb commits at source.squeak.org <commits at source.squeak.org>:
A new version of HelpSystem-Core was added to project The Inbox:
http://source.squeak.org/inbox/HelpSystem-Core-ct.123.mcz

==================== Summary ====================

Name: HelpSystem-Core-ct.123
Author: ct
Time: 13 October 2019, 9:04:08.373932 pm
UUID: dec7ceca-320f-d945-8d2a-c2f6a5e49a52
Ancestors: HelpSystem-Core-ct.120

Refactors HelpBrowser menu: Move menu stuff from HelpBrowser into HelpTopic hierarchy in favor of a better object design

Thanks again, Marcel :-)

=============== Diff against HelpSystem-Core-ct.120 ===============

Item was added:
+ ----- Method: AbstractHelpTopic>>browseTopicFromParent: (in category 'tools') -----
+ browseTopicFromParent: parentTopic
+
+ self canBrowseTopic
+ ifTrue: [^ self browseTopic].
+ parentTopic canBrowseSubtopic
+ ifTrue: [^ parentTopic browseSubtopic: self].
+ !

Item was added:
+ ----- Method: AbstractHelpTopic>>canBrowseSubtopic (in category 'testing') -----
+ canBrowseSubtopic
+
+ ^ false!

Item was added:
+ ----- Method: AbstractHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ false!

Item was added:
+ ----- Method: AbstractHelpTopic>>canBrowseTopicFromParent: (in category 'testing') -----
+ canBrowseTopicFromParent: parentTopic
+
+ ^ self canBrowseTopic or: [
+ parentTopic ifNotNil: #canBrowseSubtopic ifNil: [false]]!

Item was added:
+ ----- Method: AbstractHelpTopic>>topicMenu:parentTopic: (in category 'menus') -----
+ topicMenu: aMenu parentTopic: parentTopic
+
+ aMenu
+ add: 'Inspect (i)' translated target: self action: #inspect;
+ add: 'Explore (I)' translated target: self action: #explore.
+ (self canBrowseTopicFromParent: parentTopic)
+ ifTrue: [
+ aMenu add: 'Browse (b)' translated
+ target: self
+ selector: #browseTopicFromParent:
+ argumentList: {parentTopic} ].
+
+ ^ aMenu!

Item was added:
+ ----- Method: AbstractHelpTopic>>topicMenuKey:fromParent: (in category 'menus') -----
+ topicMenuKey: aChar fromParent: parentTopic
+
+ aChar
+ caseOf: {
+ [$b] -> [(self canBrowseTopicFromParent: parentTopic)
+ ifTrue: [ self browseTopicFromParent: parentTopic ]].
+ [$i] -> [self inspect].
+ [$I] -> [self explore] }
+ otherwise: [^ false].
+ ^ true!

Item was added:
+ ----- Method: ClassAPIHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!

Item was added:
+ ----- Method: ClassBasedHelpTopic>>canBrowseSubtopic (in category 'testing') -----
+ canBrowseSubtopic
+
+ ^ true!

Item was added:
+ ----- Method: ClassBasedHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!

Item was added:
+ ----- Method: DirectoryBasedHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!

Item was added:
+ ----- Method: FileBasedHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!

Item was removed:
- ----- Method: HelpBrowser>>browseTopic (in category 'actions') -----
- browseTopic
-
- ^ (self currentTopic respondsTo: #browseTopic)
- ifTrue: [self currentTopic browseTopic]
- ifFalse: [self currentParentTopic browseSubtopic: self currentTopic]!

Item was removed:
- ----- Method: HelpBrowser>>canBrowseTopic (in category 'testing') -----
- canBrowseTopic
-
- ^ (self currentTopic respondsTo: #browseTopic)
- or: [self currentParentTopic respondsTo: #browseSubtopic:]!

Item was removed:
- ----- Method: HelpBrowser>>exploreTopic (in category 'actions') -----
- exploreTopic
-
- ^ self currentTopic explore!

Item was removed:
- ----- Method: HelpBrowser>>inspectTopic (in category 'actions') -----
- inspectTopic
-
- ^ self currentTopic inspect!

Item was changed:
----- Method: HelpBrowser>>treeKey:from:event: (in category 'menus') -----
treeKey: aChar from: aView event: anEvent

anEvent anyModifierKeyPressed ifFalse: [^ false].
+ ^ (self currentTopic topicMenuKey: aChar fromParent: self currentParentTopic)!
- aChar
- caseOf: {
- [$b] -> [self browseTopic].
- [$i] -> [self inspectTopic].
- [$I] -> [self exploreTopic]. }
- otherwise: [^ false].
- ^ true!

Item was changed:
----- Method: HelpBrowser>>treeListMenu: (in category 'menus') -----
treeListMenu: aMenu


+ ^ self currentTopic
+ ifNil: [aMenu]
+ ifNotNil: [:topic | topic
+ topicMenu: aMenu
+ parentTopic: self currentParentTopic]!
- self currentTopic ifNil: [^ aMenu].
-
- aMenu
- add: 'Inspect (i)' action: #inspectTopic;
- add: 'Explore (I)' action: #exploreTopic.
-
- self canBrowseTopic ifTrue: [
- aMenu
- addLine;
- add: 'Browse (b)' action: #browseTopic].
-
- ^ aMenu!

Item was added:
+ ----- Method: MethodListHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!

Item was added:
+ ----- Method: PackageAPIHelpTopic>>canBrowseTopic (in category 'testing') -----
+ canBrowseTopic
+
+ ^ true!


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20191016/7de1e706/attachment-0001.html>