Added notifications on install/update and exit survey on uninstall #80

tdulcet · 2022-10-20T10:57:22Z

Added notifications on install and update.
- Clicking the update notification will open the release notes/changelog on AMO/ATN.
Added option to disable desktop notifications.
Added exit survey on uninstall.
- The draft exit survey is currently here: https://forms.gle/P8ThPXAvbGEkshYDA. Once you approve it, I can move it to https://cryptpad.fr/form/ as requested.
- Edit: It is now on CryptPad: https://cryptpad.fr/form/#/2/form/view/TyXGnnNjCOo+iC2qrvPzfO8NMK4jMg3pRKxL0mrYNs8/
Improved support for large numbers when converting fractions and constants to Unicode characters.
Updated to generate optimized RegExes from autocorrections using a Trie tree.
- This should both improve the performance of the autocorrect feature and reduce memory usage.
Bumped the version to 0.6

Per our discussion in TinyWebEx/RandomTips#9, the Notifications.js module will need to be refactored to support clicking on the notifications. For now I added this functionality to the new InstallUpgrade.js module (adapted from the Awesome Emoji Picker) to show the necessarily changes.

TODO:

Localize new strings. @rugk
Refactor Notifications module as needed.
Move exit survey to https://cryptpad.fr/form/.
Update the permissions.md file.

I believe this along with #72 should finally allow us release v1.0.

rugk · 2022-10-25T23:08:06Z

Once you approve it, I can move it to https://cryptpad.fr/form/ as requested.

AFAIK you can do the same "work together workflow" or so there, too, so you can just invite me by mail or link or whatever. Then one can just adjust it online as needed.

But good idea in general, as said.

rugk

The approach LGTM and seems all-right.

There were some minor comments, mostly code-style, and what you said indeed needs to be done before merging here.

BTW did, you know, you can checkout anything in git submodules? So you could checkout any dev version of the Notifications module or so, that should work very well for testing. (It's kinda like another git repo in your main repo.)

rugk · 2022-10-25T23:09:22Z

scripts/manifests/firefox.json

@@ -37,7 +37,8 @@
    "storage",
    "<all_urls>",
    "menus",
-    "tabs"
+    "tabs",
+    "notifications"


The permissions.md file in assets needs to be adjusted to include an explanation of why we need that permission.

It is fair to use it for the proposed use case though, I guess.

src/background/background.html

src/background/modules/InstallUpgrade.js

rugk · 2022-10-25T23:17:43Z

src/background/modules/InstallUpgrade.js

+        Notifications.showNotification(`🎉 ${manifest.name} installed`, `Thank you for installing the “${manifest.name}” add-on!\nVersion: ${manifest.version}\n\nOpen the options/preferences page to configure this extension.`);
+        break;
+    case "update":
+        if (Notifications.SEND) {


I don't really get what's the SEND variable for? Where is it modified and why cannot you just call a function?

It alos needs a better name if it is there to say, what does it send where/how/when? What does it even control?
I'd guess it goes into some shouldSendNotification direction or so,

Also again clran code-like I'd say either early return here (which may indeed be inconvenient here, if more upgrade code/logic is added, or make that whole send functionality or so into another extra function – or even file/"module", if needed.

The SEND variable is from the Notifications module. Specifically, it is the value of the new option I added to disable desktop notifications (i.e. whether to "send" the notifications).

Anyway, this is the section I suspected you would want to refactor into the existing Notifications module as part of TinyWebEx/RandomTips#9, but first we would of course need to agree on a new interface for that module. Ideas are welcome...

src/common/modules/data/DefaultSettings.js

rugk · 2022-10-25T23:24:29Z

src/options/options.html

+					<li>
+						<div class="line">
+							<input class="setting save-on-change" type="checkbox" id="send" data-optiongroup="notifications" name="send">
+							<label for="send">Display Desktop notifications</label>


Suggested change

<label for="send">Display Desktop notifications</label>

<label for="send">Display notifications on extension installation and upgrade.</label>

The user is otherwise questioned with what is shown in these notifications or whats even triggers them.

Also would not say desktop, that is an irrelevant implementation detail and may not even be true in case you manage to run this code on Android ;P

As I mentioned above, this is for all notifications, including both these "install and update" and the Thunderbird specific notification added in #72.

Note that the installation notification would not actually be possible to disable, as it would have of course already been shown by the time the user opened the options page. The upgrade notifications should be extremely infrequent, so I doubt users would want to disable them. However, users may want to disable any tip notifications added as part of TinyWebEx/RandomTips#9, which is why I added this option.

and the Thunderbird specific notification added in #72.

This is an error shown based on a user-initiated action, as such IMHO, it should not be able to be disabled, i.e. not controlled by that option, but always be shown.

Note that the installation notification would not actually be possible to disable, as it would have of course already been shown by the time the user opened the options page

Of course, fair point hehe… yeah, then we can ignore that.

Though technically, it would be possible to implement it like that and I would also do so, because there may be an edge-case where that is not to be shown: When you have installed and configured the extension and you connect a new Firefox to Firefox Sync, and your sync runs, it may happen that the extension settings/data is already syncronized, before the extension is installed? Though I don't know how Firefox handles that and in what order stuff is syncronized, so well… it also does not matter. Just keep it simple, you know.

This is an error shown based on a user-initiated action, as such IMHO, it should not be able to be disabled

OK, that is fine with me and it would be an easy change.

Though technically, it would be possible to implement it like that and I would also do so, because there may be an edge-case where that is not to be shown

Good point. It actually is already implemented like this, so your scenario if the sync ran before the new install, the installation notification would not show. Is there any other specific change you would like me to make here?

tdulcet · 2022-10-26T11:12:24Z

AFAIK you can do the same "work together workflow" or so there, too, so you can just invite me by mail or link or whatever. Then one can just adjust it online as needed.

OK, I have never used https://cryptpad.fr/form/, so that is good to know. I will look into moving the exit survey there... In the meantime, I sent you an invitation to the draft on Google Forms.

rugk

BTW that autocorrect refactoring should have been a new PR, so…

It's also kinda complicated you see…

I tried to think my way around it, but still don't see how/why that generates a more optimized RegEx. That should have been explained/stated somewhere, either in the git commit or better maybe in the source code. After all, this whole commit adds a lot of logic and complication to the source code and this has to have a good reason.

src/background/modules/AutocorrectHandler.js

rugk · 2022-12-25T10:16:25Z

src/background/modules/AutocorrectHandler.js

+        if (char) {
+            const achar = char.replace(regExSpecialChars, "\\$&");
+
+            if (!("" in obj[char] && Object.keys(obj[char]).length === 1)) {


Is "" in obj[char] the leaf note from the function below? If so, maybe use a JS Symbol or other more unique and properly named indicator.

Yes, the leaf node. I used the empty string, as it is of course not possible to have a null character in an autocorrection, but a Symbol should also be fine.

So… can we change that? Magic strings, especially unnamed ones such as an empty string, are IMHO not clean code.

I thought the use of the empty string here was clever and simplified the code (I guess I prefer simplicity over cleanliness), but yes, we can of course change it to a Symbol if you want...

Well the concept of clean code says code is read like dunno 10x more than written, so I'd always advocate for cleanliness (or better: readability) over some "it's simpler to write this way" or "it is a clever hack" or so. It may be, but in 2 weeks you have forgotten 😉

Does not have to be a symbol, but can be something else, e.g. a helper function isLeafNote and then you have much more readable code:

if (!isLeafNote(obj[char])) {

Actually, I would maybe even prefer this over a Symbol as I am writing this.

Yes, I generally agree with you. This function of course implements a complex algorithm, so it is already somewhat confusing. However, up to a point, simplicity usually improves readability (and performance), as it reduces the amount of code one needs to read and understand.

Anyway, this line actually checks for two things, that the next node is a leaf node and that there are no other sub nodes, which means it does not need to recurse any further down the tree. Replacing it with a helper function therefore is not practical, but I added the requested JS Symbol and made another change which should hopefully improve readability.

src/background/modules/AutocorrectHandler.js

rugk · 2022-12-25T10:19:57Z

src/background/modules/AutocorrectHandler.js

+
+    let result = alt.length === 1 ? alt[0] : `(?:${alt.join("|")})`;
+
+    if ("" in obj) {


uhm why?

This is really hard to understand, maybe either rexpand the JSDOC description of a high-level overview of the algorithm or somehow refactor it.
(To brainstorm: Maybe we can e.g. have an actual class TriTree with operators on it. Somehow leaf or children should be named or so.)

This checks if the current node in the tree is a leaf node, although I am somewhat abusing the "leaf" term. It usually means a node with no children (like a 🍁 in a 🌳), but I am referring a node that marks the end of one or more autocorrections.

In a previous iteration of this function for my Server Status and Link Creator add-ons, I actually used "leaf" for the key (see here), which may have been more readable, but it creates a problem since the string "leaf" could be part of an autocorrection.

An overview of the Trie algorithm can be found on Wikipedia: https://en.wikipedia.org/wiki/Trie. I think a JS Class is a bit excessive for a relatively simple function that is only used once, but I am of course open to ideas of how to make it more understandable.

Yeah it is used only once now and the linked JS class was quite simple (though I'd strip unused functions), but it's fine if we get it more readable inside this function.

rugk · 2022-12-25T10:28:19Z

src/background/modules/AutocorrectHandler.js

+function createRegEx(arr) {
+    const tree = {};
+
+    for (const s of arr.sort((a, b) => b.length - a.length)) {


This creates the three? Then maybe extract it into createTree. As mentioned above, maybe a JS-class trieThree wold be useful.

I found this example, which looks quite clean and could be used (or adapted) if the license is clear: https://gist.github.com/tpae/72e1c54471e88b689f85ad2b3940a8f0 (see also the comments which look more optimized)

(maybe we could remove the parts we do not need of course)

More inspiration:

TS implementation: https://github.com/leekevinyg/js-trie

learning article: https://dev.to/namdx1987/js-simple-trie-implementation-n23

I could rename this function to createTree as suggested and then rename the above pattern function to createRegEx, if you think that would be more clear.

Those examples just create a Trie tree and not the needed RegEx. They also do not handle the intricacies of our use case. Specifically, we need to create the Trie tree in reverse, as the resulting RegEx is right anchored since we are matching the text before the carrot as the user is typing.

For reference, my pattern function was loosely adapted from this Python code: https://gist.github.com/EricDuminil/8faabc2f3de82b24e5a371b6dc0fd1e0, which I ported to JS and heavily refactored/optimized.

tdulcet · 2022-12-25T14:12:29Z

BTW that autocorrect refactoring should have been a new PR, so…

Yes, sorry, I wrote this code a couple of months ago and I was going to create a new PR after this one was merged. However, I decided to push it here before I forgot since this PR was still waiting for your review and the changes do not add any functionality...

I tried to think my way around it, but still don't see how/why that generates a more optimized RegEx.

I hope my above comments explained it, but I am happy to answer any additional questions.

rugk

Yeah, that tree thing looks much cleaner now IMHO!

src/background/modules/InstallUpgrade.js

src/common/modules/Notifications.js

src/background/modules/InstallUpgrade.js

rugk · 2023-07-04T22:59:51Z

scripts/manifests/firefox.json

@@ -40,7 +40,7 @@
    "tabs",
    "notifications"
  ],
-  "applications": {


Are you are sure this works? Is not that manifest v3 already? (manifest_version is still 2)

Yes, applications is deprecated in MV2 and produces a warning with the linter when uploading extensions to AMO/ATN.

src/background/modules/AutocorrectHandler.js

tdulcet · 2023-07-05T10:15:23Z

As requested, I moved the draft exit survey to CryptPad: https://cryptpad.fr/form/#/2/form/view/TyXGnnNjCOo+iC2qrvPzfO8NMK4jMg3pRKxL0mrYNs8/. I will send you an e-mail with the link to edit it.

rugk

Nothing serious, lgtm.

src/common/modules/Notifications.js

Co-authored-by: rugk <rugk+git@posteo.de>

Added notifications on install and update.

7d2bced

tdulcet requested a review from rugk October 20, 2022 10:57

rugk reviewed Oct 25, 2022

View reviewed changes

tdulcet added 2 commits December 22, 2022 02:44

Improve support for large numbers when converting to Unicode characters.

3413146

Updated to generate optimized RegEx from autocorrections.

c0e41f5

tdulcet requested a review from rugk December 22, 2022 11:18

rugk requested changes Dec 25, 2022

View reviewed changes

Made requested changes.

a559705

tdulcet requested a review from rugk December 31, 2022 10:46

Updated ATN description about #73.

accad4b

rugk requested changes Feb 13, 2023

View reviewed changes

src/background/modules/InstallUpgrade.js Show resolved Hide resolved

src/common/modules/Notifications.js Outdated Show resolved Hide resolved

tdulcet mentioned this pull request Feb 23, 2023

Addon not to be found in Firefox addon store #81

Closed

tdulcet requested a review from rugk May 19, 2023 12:31

Updated to version 0.6.

1b3c8d3

catana-corp bot reviewed Jun 24, 2023

View reviewed changes

src/background/modules/InstallUpgrade.js Outdated Show resolved Hide resolved

src/background/modules/InstallUpgrade.js Outdated Show resolved Hide resolved

Added Symbol for leaf nodes.

9a17939

tdulcet force-pushed the notifications branch from 58808be to 9a17939 Compare June 26, 2023 12:14

rugk reviewed Jul 4, 2023

View reviewed changes

Made requested changes to Notifications module.

c85b06e

tdulcet requested a review from rugk July 5, 2023 10:15

rugk previously approved these changes Jul 23, 2023

View reviewed changes

src/common/modules/Notifications.js Outdated Show resolved Hide resolved

src/common/modules/Notifications.js Outdated Show resolved Hide resolved

Apply suggestions from code review

9eb3b8b

Co-authored-by: rugk <rugk+git@posteo.de>

tdulcet dismissed rugk’s stale review via 9eb3b8b July 24, 2023 09:11

Added link to CryptPad exit survey.

97d14e6

tdulcet requested a review from rugk July 28, 2023 13:06

rugk approved these changes Jul 29, 2023

View reviewed changes

rugk merged commit b3943b0 into main Jul 29, 2023
9 checks passed

rugk deleted the notifications branch July 29, 2023 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added notifications on install/update and exit survey on uninstall #80

Added notifications on install/update and exit survey on uninstall #80

tdulcet commented Oct 20, 2022 •

edited

Loading

rugk commented Oct 25, 2022

rugk left a comment

rugk Oct 25, 2022

rugk Oct 25, 2022

tdulcet Oct 26, 2022

rugk Oct 25, 2022

tdulcet Oct 26, 2022

rugk Dec 22, 2022

tdulcet Dec 23, 2022 •

edited

Loading

tdulcet commented Oct 26, 2022

rugk left a comment

rugk Dec 25, 2022

tdulcet Dec 25, 2022

rugk Feb 15, 2023

tdulcet Feb 16, 2023

rugk Jun 25, 2023

tdulcet Jun 25, 2023

rugk Dec 25, 2022

tdulcet Dec 25, 2022

rugk Dec 30, 2022

rugk Dec 25, 2022

tdulcet Dec 25, 2022

tdulcet commented Dec 25, 2022

rugk left a comment

rugk Jul 4, 2023

tdulcet Jul 5, 2023

tdulcet commented Jul 5, 2023

rugk left a comment

	<label for="send">Display Desktop notifications</label>
	<label for="send">Display notifications on extension installation and upgrade.</label>


		let result = alt.length === 1 ? alt[0] : `(?:${alt.join("\|")})`;

		if ("" in obj) {

Added notifications on install/update and exit survey on uninstall #80

Added notifications on install/update and exit survey on uninstall #80

Conversation

tdulcet commented Oct 20, 2022 • edited Loading

rugk commented Oct 25, 2022

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdulcet Dec 23, 2022 • edited Loading

Choose a reason for hiding this comment

tdulcet commented Oct 26, 2022

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdulcet commented Dec 25, 2022

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdulcet commented Jul 5, 2023

rugk left a comment

Choose a reason for hiding this comment

tdulcet commented Oct 20, 2022 •

edited

Loading

tdulcet Dec 23, 2022 •

edited

Loading