Lukas Renggli a écrit : Hi Lukas, > I have a couple of ideas below. > > >> CodeHolderSettings class>>browsersHaveAnnotationPaneSetting >> <setting> >> ^ (SettingManager newSetting: 'Show annotation pane' translated) >> parent: #codeBrowsingSettingNode; >> default: false; >> target: CodeHolder; >> setSelector: #browsersHaveAnnotationPane:; >> getSelector: #browsersHaveAnnotationPane; >> description: 'If checked then the annotation pane is shown >> in browsers; it is dynamically updated with useful informations about >> the code which is currently browsed' translated >> > > 1. The pragma <setting> seems to be a bit short and quite a common > name. Could be that others use this pragma already. Maybe something > slightly longer like <systemsettings> would be better? > ok, but note that you can use any other keyword. The default is actually setting, it can be systemsetting. you can also decide to use your own keyword for your project or your application. As an example, <seaside>, and then, browse them with SystemSettingBrowser browse: 'seaside' "for seaside settings only" or SystemSettingBrowser browse: 'seaside|systemsetting' "for seaside and systemsetting settings" > 2. To avoid having class references from everywhere onto the system > preference package I suggest that you pass into the method a builder > object (see configurations in Seaside 3.0). Like this there are no > dependencies and no compiler warnings. This would also allow to define > multiple preferences in one method. > I would'nt do that . I think it is better to have a specific package for the settings. Then, SettingManager is only referenced by packages which are dedicated to settings management. It makes them very easily removable. see also my answer for you 'after refactoring' example. > 3. getSelector:/setSelector: should be replaced by #selector:, the > write-accessor in most cases can be inferred from the read-accessor > (see Magritte). The target can be predefined to the receiving class, > that simplifies the definition. > done, but I would keep also getSelector:/setSelector: > 4. Why is there a #default:? Normally a package should already have > initialized the setting, or does it lazily when the accessor is > performed the first time. > the default is optional. Do you think it is really a problem to keep it ? > 5. I don't know what #parent: means? If this is to group the settings > of a particular application I suggest to do that in the pragma. > now we have a tree of settings (group allow only one level). parent is the selector of the method which implements the parent setting (the parent node in the tree). I think it is better like this because : - you can use sender/implementor - when I implement a setting, I don't want to know/indicate the full path > After this "refactoring" I imagine that the code looks like this: > > CodeHolder class>>systemSettings: aBuilder > <systemsettings: 'Code Browser'> > > aBuilder newSetting > label: 'Show annotation pane' translated; > selector: #browsersHaveAnnotationPane; > description: 'If checked then the annotation pane is shown ...'. > aBuilder newSetting > "defines more settings for that group" > no, this is not like that because a setting declaration is never stored (I mean in a global tree somewhere) the tree is always dynamically built (no change management is to be implemented). - one method = one setting - a setting method returns the setting declaration (which is never stored but only used dynamically by the settings browser)
> Note, that I would put the code directly on the class side of > CodeHolder. I don't see a reason why CodeHolderSettings are required. > If the preferences are stored in class variables one can easily access > the settings. > Again one can decide to remove the system-settings package and then all setting declarations. So, I think is it better to have two packages. Another reason is that a setting class variable could be defined only for the need of the setting browser. If it is declared in a setting specific package, then the system can remain clean if the setting browser is removed. thanks, thanks for your comments !!! Cheers Alain > Cheers, > Lukas > > _______________________________________________ Pharo-project mailing list [email protected] http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
