Skip to content
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

Merged
merged 4 commits into from
Sep 25, 2023
Merged

Added Emoji Autocorrect. #93

merged 4 commits into from
Sep 25, 2023

Conversation

tdulcet
Copy link
Contributor

@tdulcet tdulcet commented May 21, 2020

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.

@rugk
Copy link
Owner

rugk commented May 24, 2020

Hi @tdulcet,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

And wow, thanks for implementing this huge feature! 😃
I'll take a loot at this in the next days and will review/test it, if I get the time.

This would also be the only working add-on, for either Firefox or Chrome, to provide this feature.

That sounds good. 😉
But well… this add-on is also not for Chrome yet hehe, so see #54 for reference. 😉

Edit: fix things

@tdulcet tdulcet mentioned this pull request May 28, 2020
Copy link
Owner

@rugk rugk left a 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)

src/_locales/en/messages.json Outdated Show resolved Hide resolved
scripts/manifests/firefox.json Outdated Show resolved Hide resolved
@rugk
Copy link
Owner

rugk commented Jul 1, 2020

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.

Copy link
Owner

@rugk rugk left a 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 (like shortcodeReplace).
  • UX of the settings: autocorrectSymbols and autocorrectUnicodeFracts 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 as autocorrectUnicodeFracts 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:
grafik

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. 😃

README.md Outdated Show resolved Hide resolved
"autocorrections": autocorrections,
"longest": longest,
"symbolpatterns": symbolpatterns,
"apatterns": apatterns,
Copy link
Owner

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?

Copy link
Contributor Author

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",
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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…)

Copy link
Contributor Author

@tdulcet tdulcet Jul 22, 2020

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.

src/common/modules/data/Symbols.js Outdated Show resolved Hide resolved
src/common/modules/data/Symbols.js Outdated Show resolved Hide resolved
src/content_scripts/autocorrect.js Outdated Show resolved Hide resolved
src/content_scripts/autocorrect.js Show resolved Hide resolved
src/content_scripts/autocorrect.js Outdated Show resolved Hide resolved
src/common/modules/AutocorrectHandler.js Outdated Show resolved Hide resolved
src/common/modules/AutocorrectHandler.js Outdated Show resolved Hide resolved
@rugk
Copy link
Owner

rugk commented Jul 21, 2020

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.

@tdulcet
Copy link
Contributor Author

tdulcet commented Jul 22, 2020

@rugk Thanks for the very detailed review. I merged all the changes from your tdulcet-master branch. Also, thanks for tips, I will keep those in mind.

  • UX of the settings: autocorrectSymbols and autocorrectUnicodeFracts is not unique. Both mention ¼ as an example. So

The autocorrectSymbols replaces 1/4 with ¼, while the autocorrectUnicodeFracts replaces 0.25 (or any number that ends in .25) with ¼. I am not sure what change you would like me to make.

  • 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.

I am not able to reproduce this.

The biggest issue in my testing was, however, that it did not work. As a quick test I see it throws an exception here:

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:
image

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.

@tdulcet tdulcet requested a review from rugk August 7, 2020 10:36
@rugk
Copy link
Owner

rugk commented Aug 7, 2020

Oh sorry, missed your comment. Thanks for re-requesting the review, I'll have a look at this soon.

The autocorrectSymbols replaces 1/4 with ¼, while the autocorrectUnicodeFracts replaces 0.25 (or any number that ends in .25) with ¼. I

Ah, now I get it, that was not at all obvious.
But as we discussed both features have nothing to do with emojis per se… So I'd say this is out of scope for this project, I'm sorry. That said, as said, maybe an extra add-on just for replacing fractions and so on could be nice.

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.

Yes the demo works and yes I reloaded. I'll need to have a look again at what is the cause here…
Let's see whether that exception (that would then be shown in the console) still occurs.

Copy link
Owner

@rugk rugk left a 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.

src/content_scripts/autocorrect.js Outdated Show resolved Hide resolved

setSettings(autocorrect);

browser.runtime.onMessage.addListener((message, sender, sendResponse) => {
Copy link
Owner

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?

Copy link
Contributor Author

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

Copy link
Owner

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. 😃

Copy link
Contributor Author

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.

fracts = message.fracts;
autocorrections = message.autocorrections;
longest = message.longest;
symbolpatterns = message.symbolpatterns;
Copy link
Owner

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:

  1. 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.")
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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).

Copy link
Owner

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… 😄

@rugk
Copy link
Owner

rugk commented Oct 1, 2020

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:
case-sensitive.patch.txt

* @param {string} atext
* @returns {void}
*/
function insertCaret(target, atext) {
Copy link
Owner

@rugk rugk Oct 1, 2020

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?

@rugk
Copy link
Owner

rugk commented Oct 1, 2020

BTW you can also find your branch that I could push in my repo as tdulcet-master.

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 2, 2020

But as we discussed both features have nothing to do with emojis per se… So I'd say this is out of scope for this project, I'm sorry.

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.

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".

That said, as said, maybe an extra add-on just for replacing fractions and so on could be nice.

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.

@rugk
Copy link
Owner

rugk commented Oct 16, 2020

was also considering creating another PR to do Unicode font conversions that could instead be included in this new add-on.

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…
We certainly need to split and separate that code. I 𝑟𝑒𝑎𝑙𝑙𝑦 like 𝐭𝐡𝐞𝐬𝐞 features, but hey… they do not belong to this add-on.

I doubt users would install one add-on for Emoji autocorrect, one for Unicode smart quotes autocorrect and another one for Unicode fractions autocorrect.

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:

  • We have a special context menu just for Unicode stuff (not everything in the emoji thing)
  • You can install/use one of these things while not needing to install the other one.

Would you have any interest in creating this add-on?

Absolutely.
However, let's discuss that in a new issue. (we could e.g. cross-link the add-ons etc.)
I've created #106 for this.

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.

@rugk
Copy link
Owner

rugk commented Oct 16, 2020

For this PR:

  • the shortcode autocompletion setting is problem, because of false-positives: Elements (formerly Riot) uses :hugging: for 🤗 and then does not display the emoji if you type it in there (GitHub uses :hugs:, but displays it properly at least).
    Suggestion: I'd suggest to disable that by default.
  • **test:** we want to highlight sth. is problematic to write in Markdown, because it ends up as **test😗* we want to highlight sth. (false-positive again)

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 17, 2020

You already included this in your ZIP version… 🙈

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.

We certainly need to split and separate that code.

Only the Unicode/emoji autocorrect is currently in this PR.

However, funny Unicode font changes do have nothing to do with this emoji picker/emojis.

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.

However, let's discuss that in a new issue.

I take your point and will move this discussion to #106.

  • the shortcode autocompletion setting is problem, because of false-positives: Elements (formerly Riot) uses :hugging: for 🤗 and then does not display the emoji if you type it in there (GitHub uses :hugs:, but displays it properly at least).

It gets the :colon: shortcodes from Emoji Mart, so you will need to use those regardless of the website you are on for both the autocompletion and autocorrection to work. So for this emoji it is :hugging_face:, although all you will need to type is :hug for it to autocomplete and then any other character, such as a space (␣) or enter (↵), for it to autocorrect to the emoji.

  • **test:** we want to highlight sth. is problematic to write in Markdown, because it ends up as **test😗* we want to highlight sth. (false-positive again)

You can always press backspace (⌫) to undo any unwanted autocorrections. The :* is obviously a very common emoticon, so I am not sure if there is anything else we can do about this.

@rugk
Copy link
Owner

rugk commented Oct 17, 2020

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.

I totally agree. That's why I'd also recommend a new add-on for that, see the other issue.

You can always press backspace (⌫) to undo any unwanted autocorrections.

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. 🤔

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 18, 2020

I know, but it's still annoying, particularly because I e.g. often write these Markdown things.

The colon is problematic since it is used in most of the emoticons and obviously all the :colon: shortcodes. If you can avoid typing non-whitespace characters after colons, you should be able to prevent most false positives. Maybe you could type **test**: or **test: ** instead.

Maybe have one separate category for the "may cause false-positives" option.

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 :* emoticon to it, although as I said, it is a very common emoticon and is supported by Emoji Mart, so most users will want it enabled by default. Are there any other emoticons that you think may cause false-positives?

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.

@rugk
Copy link
Owner

rugk commented Oct 19, 2020

Maybe you could type test: or **test: ** instead.

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.

This is what I was trying to do with my "Autocorrect single character emojis" option, which you had me remove in 78a023c. I

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:

option and then add the :* emoticon to it, although as I said, it is a very common emoticon, so most users will want it enabled by default. Are there any other emoticons that you think may cause false-positives?

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.
What other emojis may need top be added to that list is a thing we'll see only after more testing, unfortunately.

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.

Yeah, basically anything with Unicode I'd say…

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 19, 2020

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.

True. We could allow the user to select/unselect each emoticon they wanted to autocorrect, but that may over complicate the options page.

What other emojis may need top be added to that list is a thing we'll see only after more testing, unfortunately.

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?

Yeah, basically anything with Unicode I'd say…

OK, will do.

@rugk
Copy link
Owner

rugk commented Oct 19, 2020

True. We could allow the user to select/unselect each emoticon they wanted to autocorrect, but that may over complicate the options page.

Also already thought of this, but that is also technically a huge burden.
The UI problem could be solved by having a "popup" (overlay) to be opened, but that alone is another thing we would have to implement.
A thing I could actually consider reasonable for now, may be a simple text input field (multi-line, one entry per line), where you can somehow enter the problematic strings (in a kind of blacklist-mode), which are always ignored.
That would make it possible to ignore those you do not want to use.
However, the problem there would be: We can hardly offer "opt-in's", i.e. of course we could add :* to that list by default, so it is excluded. But that would somehow not really look good.
Maybe a "select-list" (Enabled emojis/autocorrections") on the left and a switch button to the right, into a list "Disabled emojis"?
Yet again, harder to implement, but possibly worth it. If it has a search feature on each side (or even a combined one? May be more useful… huh…), this list does not even need a proper height.

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.)

@rugk
Copy link
Owner

rugk commented Oct 19, 2020

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… 🙃

@tdulcet
Copy link
Contributor Author

tdulcet commented Oct 20, 2020

Maybe a "select-list" (Enabled emojis/autocorrections") on the left and a switch button to the right, into a list "Disabled emojis"?
Yet again, harder to implement, but possibly worth it. If it has a search feature on each side (or even a combined one? May be more useful… huh…), this list does not even need a proper height.

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.

rugk
rugk previously approved these changes Jun 1, 2021
Copy link
Owner

@rugk rugk left a 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. 😄

@rugk
Copy link
Owner

rugk commented Jun 12, 2022

@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… 👀

@tdulcet
Copy link
Contributor Author

tdulcet commented Jun 13, 2022

@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.

@rugk
Copy link
Owner

rugk commented Jun 14, 2022

I need to go through all of these some time and merge stuff or so sorry for that delay… :|

Copy link
Owner

@rugk rugk left a 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/common/modules/PageHandler.js Outdated Show resolved Hide resolved
src/content_scripts/autocorrect.js Show resolved Hide resolved
src/content_scripts/autocorrect.js Outdated Show resolved Hide resolved
* @returns {void}
*/
function undoAutocorrect(event) {
// console.log('beforeinput', event.inputType, event.data);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead code

Copy link
Contributor Author

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.

src/content_scripts/autocorrect.js Outdated Show resolved Hide resolved
src/options/modules/CustomOptionTriggers.js Show resolved Hide resolved
src/options/modules/CustomOptionTriggers.js Show resolved Hide resolved
src/options/modules/CustomOptionTriggers.js Outdated Show resolved Hide resolved
src/options/options.html Show resolved Hide resolved
@tdulcet
Copy link
Contributor Author

tdulcet commented Aug 10, 2023

BTW this is like 90% of the same code also used in Unicodify, is not it?

Yes, see rugk/unicodify#42.

BTW did you force-push?

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

@tdulcet tdulcet requested a review from rugk August 11, 2023 15:41
Copy link
Owner

@rugk rugk left a 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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code?

Copy link
Contributor Author

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.

rugk
rugk previously approved these changes Aug 15, 2023
@rugk rugk merged commit 7379aa2 into rugk:main Sep 25, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically replace :colon: shortcodes and Emoticons as user types
2 participants