-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Emoji Autocorrect. #93
Conversation
Hi @tdulcet, And wow, thanks for implementing this huge feature! 😃
That sounds good. 😉 Edit: fix things |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a full review yet
(only reviewed 8156d54 for now)
Yes, I'm aware of this PR and sorry for the delay, but I'm currently busy. I'll for sure have a look and try to include/review it before the next release of this add-on, that is for sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'll had a look again.
First of all awesome work and you've followed the coding guide quite good.
However, please do use eslint. You e.g. used tabs, while we use spaces in the files. I've autocorrected most of these simple issued, but some may remain. (Edit: no I could not actually push my changes, due to mysterious git submodule errors, arrg:
warning: 9c20203:.gitmodules, multiple configurations found for 'submodule.src/common/modules/Localizer.path'. Skipping second one!
)
Also install/use an editorconfig plugin or so for your editor in case it is not built-in. We use that in our repo, so it should notice and use that.
As for the options:
- it's good, you have so many in a detailed way
- however, please group them under a new heading, not in the common "behaviour" one, e.g. "Shortcode replacement" – or better, "Emoji autocompletion"
- do the same internally, i.e. in
DefaultSettings.js
use a new object to group them (likeshortcodeReplace
). - UX of the settings:
autocorrectSymbols
andautocorrectUnicodeFracts
is not unique. Both mention ¼ as an example. So - UX: the "(not recommended)" should be replaced by a helper text that explains why it is not recommended, e.g. "This feature is not recommended, because it may erroneously replace characters that are not intended to be an emoji." (what we'd call false-positives, but that is a way too technical speak for such a line)
autocorrectUnicodeQuotes
is, if we'd been exact, off-topic for an emoji picker. (same asautocorrectUnicodeFracts
strictly speaking) I'd still need to consider whether we should include it. In any way, e.g. remember in German quotation marks are differently. So that would need language detection to be really accurate. As that is way to complicated, I'd tend to not have this in this add-on and rather say this would be a good idea for a new add-on that just does "smart quotes replacements".- UX: replace the "arrow" in the helper texts for the emoji examples being replaced by some more elaborate text, e.g. "For example A gets replaced with B and C with D". The abbreviations and already huge overload of emojis in these helper texts make it already very difficult to read and understand it it (at the first glance). And we have that space there for two examples being written.
- I've changed some defaults I think that should be changed and moved the permission message-box nearer to the options (keep it at the top of all options e.g. that are affected by this)
- If you disable "Autocorrect Emoticon Emojis" and decline the permission request, you cannot re-enable it. Here likely some manual disabling/reset is missing. See how other parts of the options page do it.
The biggest issue in my testing was, however, that it did not work. As a quick test I see it throws an exception here:
BTW: Next time, try to avoid doing a pull request from the master branch, because you can run into problems when you have a "non-clean" master that does not follow this repo here (i.e. "upstream"). See this article for details. Anyway, this is only a tip for the next time. 😃
"autocorrections": autocorrections, | ||
"longest": longest, | ||
"symbolpatterns": symbolpatterns, | ||
"apatterns": apatterns, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this variable name mean? What are apatterns
. Could you please use a little more self-explaining var name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are pattern to not autocorrect for. See the comment here. I can add another comment.
@@ -13,5 +13,7 @@ | |||
* @type {Object.<string, string>} | |||
*/ | |||
export const COMMUNICATION_MESSAGE_TYPE = Object.freeze({ | |||
OMNIBAR_TOGGLE: "omnibarToggle" | |||
OMNIBAR_TOGGLE: "omnibarToggle", | |||
AUTOCORRECT_BACKGROUND: "autocorrectBackground", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the difference between AUTOCORRECT_BACKGROUND
and AUTOCORRECT_CONTENT
messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AUTOCORRECT_BACKGROUND
is for communication between the options page and the autocorrect module. The AUTOCORRECT_CONTENT
is for communication between the content scripts and the autocorrect module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe name it by their intend, like AUTOCORRECT_SETTINGS_UPDATE
and AUTOCORRECT_TEXT
(if it sends text or so…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AUTOCORRECT_BACKGROUND
if for sending the updated options from the options page to the autocorrect module. The AUTOCORRECT_CONTENT
is for sending the options and the two regular expressions from the autocorrect module to the content script(s) in each tab.
Note: In addition to my comments, please also mind to use the GitHub review features, e.g. clicking "Resolve conversation" if you've fixed an issue/comment. |
@rugk Thanks for the very detailed review. I merged all the changes from your
The
I am not able to reproduce this.
I have been testing this for months in Firefox, Thunderbird and Chrome without any issues. Does my demo from #66 work for you? Did you try reloading the page? Are there any errors in either the web console or the extension's console. Here is what I get when I type what was in your screenshot: I believe I made all of your requested changes and I clicked "Resolve conversation" for everything that I was certain on. Please let me know if you would like me to make any further changes. |
Oh sorry, missed your comment. Thanks for re-requesting the review, I'll have a look at this soon.
Ah, now I get it, that was not at all obvious.
Yes the demo works and yes I reloaded. I'll need to have a look again at what is the cause here… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I was busy. Tried it out again and I still have that error that it does not work.
See one inline comment below, where I speculate a little bit.
|
||
setSettings(autocorrect); | ||
|
||
browser.runtime.onMessage.addListener((message, sender, sendResponse) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why do custom coding here. Can't we just use BrowserCommunication.addListener
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because it needs to be able to send a response and BrowserCommunication
does not support that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it should be possible: https://github.com/TinyWebEx/BrowserCommunication/blob/19e9abc44bac6724acebfe355da8aeb1e3b424c8/BrowserCommunication.js#L25-L38
sendResponseCallback
is exposed there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes, you are correct. It is needed because the message is coming from the content script, which cannot use modules like BrowserCommunication
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, anyway, this only means the content script needs to manually specify COMMUNICATION_MESSAGE_TYPE.AUTOCORRECT_CONTENT
, everything else should be compatible here. 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I tried updating it to use BrowserCommunication
, but it did not work. The response is never received by the content script.
src/content_scripts/autocorrect.js
Outdated
fracts = message.fracts; | ||
autocorrections = message.autocorrections; | ||
longest = message.longest; | ||
symbolpatterns = message.symbolpatterns; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry for the delay, I still get the symbolpatterns = null
error as mentioned above.
I guess this is because the message has not been received here. Setting some breakpoints here or so also shows me nothing is being received.
So maybe:
- add an explicit check/better error handling when sth. is not received and throw an
Error
with a better description. As that may e.g. also happen when you decline the permission on the options page AFAIK? LIke `throw new Error("emoji autocomplete settings have not been received. Do not autocomplete.") - we need to find the cause iof that error
I also tried changing the settings in the options and all other tabs did not receive an update. AFAIK they should, should not they?
I guess the error lies in AutocorrectHandler.js
somehow.
And indeed, I could fix a camel-case error there, at least – that was pointed out by EsLint. That did not fix this whole problem though… 🤔
(I#ve pushed the commit.)
So important: Please use EsLint as per our contributing guide.
Anyway, before this can get merged, I need to confirm it works and test that out. So we need to get the cause of this error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I updated it to throw your error, but this error should never happen if it is working correctly. The
tabs
permission requested on the options page is only used after the user changes the options, so it should have no effect here. - Yes, I agree! I find it very weird that it works for me in mutiple instances of Firefox, but yet it is not working for you. Could there be some Firefox setting or another extension that is interfering with it?
I also tried changing the settings in the options and all other tabs did not receive an update. AFAIK they should, should not they?
Yes, if you accepted the tabs
permission.
And indeed, I could fix a camel-case error there, at least – that was pointed out by EsLint.
If I change the emojiMart
to emojimart
, I get this error in the console:
Uncaught (in promise) TypeError: emojimart.emojiIndex is undefined
Anyway, before this can get merged, I need to confirm it works and test that out. So we need to get the cause of this error.
Could it possibly be that because of the "submodule error", not all the required dependencies have downloaded? Here is the version I have been testing (note that it also includes full Thunderbird and Chrome support, among other out of scope features).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed your ZIP version works. I'll need to investigate that further… 😄
Arrg, I cannot push to your repo, because git shows me some strange submodule errors yet again… Here is the patch file for you to apply: |
src/content_scripts/autocorrect.js
Outdated
* @param {string} atext | ||
* @returns {void} | ||
*/ | ||
function insertCaret(target, atext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdulcet Well… okay, so if we merge thuis first, I still would like a TODO
comment in here at least for unifying/"merging" the code later, so we do not forget it.
So could you please add this?
BTW you can also find your branch that I could push in my repo as |
I thought it would be nice to have one add-on handle everything related to Unicode and Unicode autocorrections. I doubt users would install one add-on for Emoji autocorrect, one for Unicode smart quotes autocorrect and another one for Unicode fractions autocorrect. In addition, most users probably do not know the difference between Unicode symbols and emojis, considering that most emojis are also symbols.
Would you have any interest in creating this add-on? I would be happy to close this PR and work with you into turning it into a separate Unicode autocorrections add-on. I was also considering creating another PR to do Unicode font conversions that could instead be included in this new add-on. I believe I made all of your requested changes in fd6beaf. |
You already included this in your ZIP version… 🙈 So I assume your ZIP does not only belong of this branch here, maybe this causes the issues…
I agree to some extend. However, funny Unicode font changes do have nothing to do with this emoji picker/emojis. As such, although I again stress I personally like this and find it very funny, – I like to stress this again – I think this does not fit into this add-on. Some other advantages for a separate add-on:
Absolutely. BTW sorry for the late reply again, I'm quite busy, but I'm trying to keep up here and do definitively have interest in cooperating for an add-on. |
For this PR:
|
Yes, as I said, it included out of scope features. It included all the features I was considering creating PRs for, so that I could easily test them all at once.
Only the Unicode/emoji autocorrect is currently in this PR.
I was only considering this feature for a possible future PR since one of the Unicode fonts uses emojis and at the time I thought this add-on could potentially handle everything related to Unicode. I was going to explain it in more detail if/when I ever created the PR, but in short there are many websites that can do the Unicode font conversion, such as https://coolfont.org, https://fsymbols.com/all/ and https://qaz.wtf/u/convert.cgi. However, they all require the user manually copy and paste their text to the website and back, which obviously has similar privacy and security implications to those that your "Offline QR Code Generator" solves. There currently are not any add-ons for either Firefox or Chrome that can do this. This feature is useful on websites that do not support changing the font and/or text formatting, including many popular social media websites like Instagram and LinkedIn. While some of the Unicode fonts may be considered "funny", particularly the one that uses emojis, this feature also has many serious and professional uses. Note that it also fixed #45.
I take your point and will move this discussion to #106.
It gets the
You can always press backspace (⌫) to undo any unwanted autocorrections. The |
I totally agree. That's why I'd also recommend a new add-on for that, see the other issue.
I know, but it's still annoying, particularly because I e.g. often write these Markdown things. So for a good usability we really need to minimize the number of false-positives. Maybe have one separate category for the "may cause false-positives" option. 🤔 |
The colon is problematic since it is used in most of the emoticons and obviously all the
This is what I was trying to do with my "Autocorrect single character emojis" option, which you had me remove in 78a023c. I could readd and rename that option and then add the Please let me know exactly what changes you would like me to make to this PR. Based on your comments and annotations in #106, I am assuming you would like me to remove the "Autocorrect Unicode Symbols", "Use Unicode smart quotes" and "Convert fractions and mathematical constants to Unicode characters" options. |
Well, a good idea for a workaround, but still does not solve the usability poroblem. We just cannot tell all users to change their habit.
Well yeah, because these were only two emojis and produced even more false-positives. And I still would not include them in such an option. However:
Yes, such an option would be good, I guess. It's unfortunate, but we'll always have such false-positives and need to weigh their popularity/usage against the false-positive rate they produce.
Yeah, basically anything with Unicode I'd say… |
True. We could allow the user to select/unselect each emoticon they wanted to autocorrect, but that may over complicate the options page.
OK, would you like me to go ahead and add that option with this single emoji now or wait until you have had a chance to do more testing?
OK, will do. |
Also already thought of this, but that is also technically a huge burden. So for a first iteration, I'd rather target the blacklist-approach. Or better: Let's first think about the UI before we try to implement it. (That's why I discuss this in such a detail here.) |
As for the review: I'll leave this in the queue for now, until I can again find time to try it out. Hopefully it works then hehe… 🙃 |
Do you mean something like this, but with the enabled emoticons on the left and the disabled ones on the right? I guess we would store the disabled ones. The tricky part would be then figuring out how to save all the disabled emoticons to persistent storage with your AutomaticSettings library, not just the selected one(s). I removed the "Autocorrect Unicode Symbols", "Use Unicode smart quotes" and "Convert fractions and mathematical constants to Unicode characters" options in 6610f89. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given I've approved it before, I only looked at the Readme changes and thanks, that LGTM. 😄
@tdulcet Does someone wants to merge in master here then to fix tnhat merge conflicts? LIkely won't help when it is stale for even longer… 👀 |
Yes, I would be happy to fix the merge conflicts. Depending on your review of rugk/unicodify#72, I could make those changes here as well. However, I have several more PRs to make after #127, which will cause more merge conflicts here, so it would probably be best to rebase this just once after those, unless you would like to merge it sooner. I can also add Thunderbird and Chrome support to this. |
I need to go through all of these some time and merge stuff or so sorry for that delay… :| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW this is like 90% of the same code also used in Unicodify, is not it? IF so, we should really try to move that into a lib. I just made a repo for that and invited you, feel free to use that: https://github.com/TinyWebEx/TextModification/
BTW did you force-push? Because GitHub says I have reviewed some files already, but I can only see one commit.
If so, please, don't do that. It just makes reviewing things way harder as I can not see your recent changes, but need to review everything again.
Such a big PR is hard to review in general anyway.
src/content_scripts/autocorrect.js
Outdated
* @returns {void} | ||
*/ | ||
function undoAutocorrect(event) { | ||
// console.log('beforeinput', event.inputType, event.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is very useful for debugging this function.
Yes, see rugk/unicodify#42.
Yeah, I had previously tried to resolve the merge conflicts to keep this PR up to date, but after I separated many of the changes into new PRs (#126, #127, #134, etc.) and with all the other commits to the master/main branch over the last 3+ years, it just became too difficult to maintain, so I decided to start this PR over from a clean state... However, you can still see the changes from my force push here, but this does include all the other changes to the main branch: https://github.com/rugk/awesome-emoji-picker/compare/306167de7c07f84d66de51cf7ad858b926d7f2c9..b6f417cd75effb5b3f170517c2818593f178e550 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor stuff
} | ||
} | ||
running = false; | ||
// console.log('beforeinput', event.inputType, event.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as line 279 below for the undo autocorrect.
:colon:
shortcodes and Emoticons with Emojis as the user types. Closes Automatically replace :colon: shortcodes and Emoticons as user types #66.You did not specify what features you would accept, so I included all six options relating to Unicode symbols/emojis and all 140 emoticon mappings from my demo from #66. It gets the
:colon:
shortcode mappings from Emoji Mart.From my testing, it seems to work on most websites. I could use help from users testing it on even more websites (#61). There are a couple bugs in Firefox for Android that are listed in the "Browser Notes" section of the demo.
It requires the
tabs
permission to send any updated options to the content scripts in each tab.I think this feature should be enabled by default, as it is much easier for the user to insert their frequently used emojis, then manually opening the emoji picker each time and it supports symbols that Emoji Mart does not. This would also be the only working add-on, for either Firefox or Chrome, to provide this feature. The user can press backspace (⌫) to undo any autocorrections. Please let me know if you would like me to make any changes.