Improve example of UI::Notifications regarding SketchupExtension argument

[thomthom]

https://ruby.sketchup.com/UI/Notification.html

Currently the examples are:

# For consistency, the accept (yes) and the dismiss (no) buttons
# are always displayed in the same order.
message = "A new version of pizza is available. Install now?"
notification = UI::Notification.new(sketchup_extension, message)
notification.on_accept("Pizza!") { puts "Pizza" }
notification.on_dismiss("No thanks") { puts "No pizza" }
notification.show

We omit how to pass the SketchupExtension argument. We should provide an example that clarifies that extensions should only use SketchupExtension.new once, from the root RB file and hold on to a reference to it if needed. (Or use Sketchup.extensions)

[DanRathbun]

I have been wondering about why the notification constructor requires a SketchupExtension instance argument.

If we wanted notifications to be linked to extensions, why not add a factory notice method to the SketchupExtension class ?

---

Yes, there are coders that may think to use a notification rather than a messagebox, and might be tempted to create a "junk" extension instance thus ...

notice = UI::Notification.new(SketchupExtension.new("Junk",""), "You cannot use that command now!")
notice.show

Doing this does not add it to the SketchUp extensions collection. (I'm not sure why, probably because this "junk" instance has not been registered.)

[thomthom]

I have been wondering about why the notification constructor requires a SketchupExtension instance argument.

So that we would have the ability to allow end users to mute notification from a given source.

If we wanted notifications to be linked to extensions, why not add a factory notice method to the SketchupExtension class ?

I guess it could have been SketchupExtension#notification - but what would that solve?

Doing this does not add it to the SketchUp extensions collection. (I'm not sure why, probably because this "junk" instance has not been registered.)

Correct, it must be registered. That being said, we're not verifying the extension instance is registered - which we could and probably should do. However, it doesn't really matter right now as we aren't using it. Abuse has become a problem so it's not been on the top of our list.

[DanRathbun]

Abuse has or has not become a problem ... ?

I guess it could have been SketchupExtension#notification - but what would that solve?

It would be more Rubyish. It "seems" (to me) poor practice to have a required instance argument, rather than an instance method of such an instance. (Where else in the API is this kind of 1st instance argument also used? [Ie, this seems more Julia-like or C-like than Rubyish.])

In addition, such an instance factory method limits use of the said class (UI::Notification) to a valid instance by it's very nature, and the factory method's parameter validation can raise an exception if the extension instance is not registered. Like other limited classes, the constructor could be hidden so as to prevent misuse.

---

ASIDE" However, there is likely to be coders who may wish to use notifications in a more "ad hoc" manner, but they really need more style controls as well as delay time setting to replace timed messages (usually implemented as web dialogs.)

[thomthom]

It would be more Rubyish. It "seems" (to me) poor practice to have a required instance argument, rather than an instance method of such an instance.

I think the pattern should rather depend on what should depend on what. UI is typically higher level than most things. To have SketchupExtension deal with UI logic would not be a good coupling IMO. Also, passing an instance into the constructor is dependency injection, common pattern, even in Ruby.

[DanRathbun]

I think the pattern should rather depend on what should depend on what.

This "play it by ear" protocol is "irregular" and may serve to confuse coders who would expect good, standard API patterns.

UI is typically higher level than most things. To have SketchupExtension deal with UI logic would not be a good coupling IMO.

Disagree. SketchupExtension would not be "dealing with UI logic". The logic still occurs in the UI::Notification class. Doing it properly (restricting instantiation and misuse) via factory methods is the established API pattern in most (if not all) API classes, as well as UI module classes.

Also, passing an instance into the constructor is dependency injection, common pattern, even in Ruby.

With respect to encapsulated Ruby processes like SketchUp's, I disagree. Perhaps for non-model "virtual" classes (ie, those in the Geom module,) but not for any of the member classes of API managed collections (ie, Sketchup::Entity subclasses.)

This implementation is a departure from the norm.

---

Now all this said. IMO this class is implemented so poorly (and still lacks font size control [ie, the font is too small,]) that I myself do not use them.

So since we disagree, and you're not likely to change things, ... I'll just continue to shun this class. And there is not anything more to say on the subject, other than that the need to actually open this issue is a fact to bolster my position. If use of notifications was done in a manufactural manner (hiding the constructor,) then there would be no need for "improving examples" and there would be no possibility of misuse.

[thomthom]

What we needed was "a way to identify what extension produces the notification". That doesn't have to be SketchupExtension. But if you make the factory of UI::Notification part of SketchupExtension then they are rightly coupled. With the current behaviour we could easily accept other means of identifying the source extension. It's a looser coupling.

With entity types they are by nature tightly coupled to their entities collection. Their relationship pattern doesn't automatically translate everywhere else.

[Eneroth3]

I agree the loose coupling is better. The Notification takes a SketchUpExtension to give us abilities such as muting extensions if needed (or maybe re-use their icon). Even if not used at this time, it gives us more options for improvements. The Notification is however not a behavior of the SketchUpExtension. It's a separate thing that just potentially benefits from knowing about the SketchUpExtension that triggered it. To me this is perfectly normal.

Regarding font size control, that could be added to the SketchUp accessibility options, but isn't really related to the API design.