new: add P suffix to pause refresh while the dropdown menu is open …#173
new: add P suffix to pause refresh while the dropdown menu is open …#173vaab wants to merge 1 commit intop-e-w:masterfrom
P suffix to pause refresh while the dropdown menu is open …#173Conversation
| * if `INTERVAL` is followed by `+`, the plugin is additionally re-run each time the dropdown menu is opened. | ||
| * if `INTERVAL` is followed by one or more of the following (order is not important): | ||
| - `+`, the plugin is additionally re-run each time the dropdown menu is opened. | ||
| - `P`, the plugin will pause (avoid refresh) as soon as the dropdown menu is opened. |
There was a problem hiding this comment.
AFAICS, + and P are mutually exclusive. I'm not sure what a combination of both achieves. Perhaps +P makes some sense (run plugin when the menu is opened, but just once), but P+ does not.
And maybe you should use some non-alpha character like - or $ instead of P.
There was a problem hiding this comment.
+P and P+ are the same, and there are not mutually exclusive, but independant:
+will request a refresh when dropdown menu is openedPwill prevent any refresh while the dropdown is opened (this doesn't conflict with+, as the the refresh is done before or at the dropdown opening).
The idea ofPis just to have a stable display of the menu once opened, especially when having submenus, because they close themselves. So it makes sense to want to refresh it before opening it also.
I feel the description I provided in the help is not clear enough, would you like me to rephrase it ? My suggestion would be:
the plugin will pause (avoid refresh) while the dropdown is open.
How do you feel about that ?
As for the alpha-character (namely P) chosen, I felt that these magick characters were quickly getting confusing as symbol, but my main concern was that any characater added here needs to be accepted nicely in a filesystem (as they must be part of the filename). And if other behavior were to be added, we will run short of symbol very quickly. I can understand that alpha can create confusion with m, s ... etc ... this is why I went for uppercase.
I'm okay to change that if this is important to you. What symbol would you like ?
| settings.updateOnOpen = true; | ||
| updatePart = updatePart.substring(0, updatePart.length - 1); | ||
| if (updatePart !== null) { | ||
| while (["+", "P"].includes(updatePart.substring(updatePart.length - 1))) { |
There was a problem hiding this comment.
This is ugly. If you need to support both P and + at the same time, you just support P+ or +P, not both. see comment above.
There was a problem hiding this comment.
I agree that +P and P+ are equivalent, as a consequence, we might enforce the usage of only one of both strings. Alas, I don't see why we should. My perspective is that each character brings a feature. The order is not important.
If you force arbitrarily one of the combination, this will require a somewhat different documentation in the README than the one I provided, and I'm not really sure how to present it in a simple and clear way. For me it breaks a simple concept.
However, I agree that the code could be better. Would this suit you better ?:
while (true) {
if (updatePart.endsWith("+")) {
settings.updateOnOpen = true;
} else if (updatePart.endsWith("P")) {
settings.pauseWhileOpen = true;
} else {
// no more recognized character suffixes
break;
}
// remove the matched character
updatePart = updatePart.substring(0, updatePart.length - 1);
}
Note:
- it still supports any order for
+andP - it simplifies the tests
- some comment were added
I'm happy to update this PR with this change if you are okay with this proposal.
|
Have you looked at #179? Does it fix your issue? |
|
Yes it does ! That's way better, I'll have a look. Many thanks for your time ! |
…(fixes #29, #143)