[background-tips] Add service API for tip registration and redistribute tips#1417
[background-tips] Add service API for tip registration and redistribute tips#1417asiloisad wants to merge 4 commits intopulsar-edit:masterfrom
Conversation
|
The "changes text in the message" test was failing because tips.js only had a single default tip, so cycling always produced the same text. Added two more core editor tips that don't depend on any specific package. |
|
This is a fantastic idea and we should look at it when we don't have fires to put out. |
confused-Techie
left a comment
There was a problem hiding this comment.
Added a super quick review based on the consumption of the service in other packages, and will have to leave a more in depth review for later.
But overall, super love this idea and how the current implementation looks! Very rad idea to do this!
There was a problem hiding this comment.
I know it might be somewhat antithetical to this PR, but we may want to leave the github package based tips in here until we are able to actually migrate that package into core, which we are still working towards and tracking in #512
| this.backgroundTipsView = new BackgroundTipsView() | ||
| activate() { | ||
| this.defaultTips = require("./tips"); | ||
| this.addedTips = new Set(); |
There was a problem hiding this comment.
I can't test this at the moment. But with storing all acquired tips in a Set what are the chances we would retain tips after a package has been disabled in our current session? If this is still an issue, we could consider that still not even loading them after a reload is a huge improvement from previous behavior, but was just curious considering the goals of this PR.
There was a problem hiding this comment.
You are right, the service is too simple now. I will prepare new commits today that will handle lifecycle of tips better.
|
Oh, I found that deferred packages (like command-palette and find-and-replace) don't register their consumed services until activation, sot their tips won't show on startup, Ironically, these are the tips most useful for new users who don't know the keybindings yet. A early-share mechanism would be useful that send tips before a delayed start. |
Oof. That's a tough one to work around. Let me think about it. EDIT: Maybe it could be part of the EDIT 2: This would require that we be able to tell the difference between a not-yet-activated package (deferred activation for performance reasons) and an explicitly disabled package — the latter should not have tips shown. I'm not 100% certain that those are separate concepts, but I'll look into it. |
|
I have in mind that service may provide additional optional flag (next to version) like "onInitialize" that package manager will recognize and deal with service before activation (but after initialization) if package activation is deferred. Consumed service of this topic is going like: "background-tips": {
"versions": {
"1.0.0": "consumeBackgroundTips"
},
"onInitialize": true
}A design of this package service need some updates then, because .disposables are not available yet, but It can be solved. |
|
In the short term, there won't be a good way to make this service work for packages that defer their activation. But the solution to that could just be to keep those packages' tips in the (As an aside: I've long felt that Another thing I thought of: I imagine the |
The
background-tipspackage previously had all tips hardcoded, including tips referencing commands from other package. This meant tips would still appear even when the referenced package was disabled or replaced — for example, a user who disablesfuzzy-finderin favor of community package would still see tips aboutfuzzy-finderkeybindings.This PR adds a
providedServicesAPI tobackground-tipsso that packages can register and unregister their own tips dynamically. When a package is disabled, its tips are automatically removed from the rotation. Community packages can also use this service to contribute their own tips.