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

Add emoji style setting #720

Merged
merged 36 commits into from
Jan 18, 2019
Merged

Add emoji style setting #720

merged 36 commits into from
Jan 18, 2019

Conversation

mikigal
Copy link
Contributor

@mikigal mikigal commented Jan 14, 2019

Add option to use old Messenger emoji. You can enable it in options.

@CvX
Copy link
Collaborator

CvX commented Jan 14, 2019

  1. Emoji replacement shouldn't really be in setUpPrivacyBlocking. That code should have its own function instead.
  2. The URL replacement algorithm is too complex because it not only changes the emoji style but also its size. If we keep the original size/pixel ratio (and I think we should), then the replacement comes down to just a single letter change, e.g. https://static.xx.fbcdn.net/images/emoji.php/v9/tf8/2/32/1f61b.png becomes https://static.xx.fbcdn.net/images/emoji.php/v9/zf8/2/32/1f61b.png ('t' -> 'z')
  3. As far as I know there are three styles available: 't' (the current default), 'z', and 'f' (e.g. https://static.xx.fbcdn.net/images/emoji.php/v9/ff8/2/32/1f61b.png). Should we offer all three?

@mikigal
Copy link
Contributor Author

mikigal commented Jan 14, 2019

I think we can offer thee types of emoji, user will can select it in menu.
I will follow your suggestions.

@mikigal
Copy link
Contributor Author

mikigal commented Jan 14, 2019

I've created menu with 3 emoji options, separated emoji code from setUpPrivacyBlocking. I haven't idea to name options so I simply name it 'F style', etc.

@CvX
Copy link
Collaborator

CvX commented Jan 16, 2019

@mikigal I've added more fixed and slightly reorganized the code. Please take a look if everything looks good to you. 🙂

Before we merge, I have some questions:

  1. Is *://*.${domain}/images/emoji.php/v9 needed here?
    const filter = {urls: ['*://static.xx.fbcdn.net/images/emoji.php/v9/*', `*://*.${domain}/images/emoji.php/v9`]};
  2. We probably should handle in some way emoji that isn't available in older emoji sets, right? Currently using the latest emoji ends up like this: screen shot 2019-01-16 at 19 04 39
  3. This one is to @sindresorhus – should menu options like this be all placed in "Caprine > Caprine Preferences"? I feel like at least some of them should be moved to e.g. "View" or "Window".

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

  1. Emoji can be fetched from facebook.com too. I'm not sure when it fetch from static.xx.fbcdn.net or facebook.com. It can't be downloaded from messenger.com. I replace {domain} to facebook.com for additional safety.

  2. I created exclude list. It is needed only for Messenger 1.0 version. For these emoji we don’t do anything and give emoji from newest version. For Facebook 2.2 everything seems to work.
    I changed default emoji style to the newest version too.

I think everything is correct now :)

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

It seem's it can be only one webRequest.onBeforeRequest. It seems second onBeforeRequest override first, and privacy blocking didn't worked, because emoji override it. I worked around it by creating one onBeforeRequest that trigger privacy blocking and emoji methods. Now everything works :)

@CvX
Copy link
Collaborator

CvX commented Jan 16, 2019

You're right about webRequest.onBeforeRequest! I just assumed it allows registering multiple listeners… Should have read the docs! 😄

I'm pushing more changes:

  • Added more emoji codes to the excludes list (Facebook 3.0 set introduced 161 new emojis), removed duplicates and sorted it alphanumerically
  • cleaned up the code and extracted it again to the separate file (who knew it would grow so much 😜)

Let me know if I broke anything again. 😉

Also, two more questions:

  1. What's the reason for these changes?

    caprine/index.js

    Lines 426 to 436 in f07aef9

    if (mainWindow !== undefined) {
    mainWindow.show();
    }
    });
    app.on('before-quit', () => {
    isQuitting = true;
    if (mainWindow !== undefined) {
    config.set('lastWindowState', mainWindow.getNormalBounds());
    }
  2. Since changing active emoji set requires app restart, should we display a message box offering a restart?

@CvX
Copy link
Collaborator

CvX commented Jan 16, 2019

Oh, and because of all those new emoji I've enabled emoji replacement for Facebook 2.2 set. Without it, the support is kinda spotty:
screen shot 2019-01-16 at 23 20 35

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

Oh, this changes was for test, because I have another problem with Caprine, and it seems to help. I committed it in mistake, sorry. I think alert with restart option is good idea. Currently I don't have idea why emoticons works in menu, but crash after send. I will check it tomorrow.

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

I don't know why, but I haven't any problems with emoji from your screenshot. Please check by devtools(F12) from what URL it tries download.

nkj22ushr1

@CvX
Copy link
Collaborator

CvX commented Jan 16, 2019

Oh, they do work now (when using fallback). I'm just saying I've enabled the excludes list for Facebook 2.2 set because these were previously missing in e.g. 8f961df and f07aef9. Or did these work for you before my recent commits?

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

It worked for me, without exclude list for Facebook 2.2. But I found something interesting. It looks, that facebook automatically redirect some emoji from FB 2.2 (I think all missing for this set) to FB 3.0. It's why I made exclude list only for 1.0
i8mq7tsvgo

@CvX
Copy link
Collaborator

CvX commented Jan 16, 2019

That's how our replacement shows up in DevTools. 🙂

@mikigal
Copy link
Contributor Author

mikigal commented Jan 16, 2019

Not every! There are a lot of downloaded emoji without any redirect, but from selected set. It's strange but it seems that facebook redirect it automatically. So does we need exclude list for FB2.0 too?
pas7wxxf80

@mikigal
Copy link
Contributor Author

mikigal commented Jan 17, 2019

@CvX Can you add comment how to get all excludes?

@CvX
Copy link
Collaborator

CvX commented Jan 17, 2019

All requested changes have been made. (shouldn't Github automatically mark the review green?)

Though I have a question to @sindresorhus that got lost in the discussion:

3. should menu options like this be all placed in "Caprine > Caprine Preferences"? I feel like at least some of them should be moved to e.g. "View" or "Window".

Anyway, that's a non-blocker. LGTM 🙂

@sindresorhus
Copy link
Owner

  1. should menu options like this be all placed in "Caprine > Caprine Preferences"? I feel like at least some of them should be moved to e.g. "View" or "Window".

Yeah, I missed that. No, Preferences is correct. "View" and "Window" should only contain transient and context-sensitive actions, meaning actions you do often and that are not meant to be permanent. You could argue the Vibrancy thing should be in Preferences too.

@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@sindresorhus sindresorhus changed the title Add old emoji option Add emoji setting Jan 17, 2019
@sindresorhus sindresorhus changed the title Add emoji setting Add emoji style setting Jan 17, 2019
menu.js Outdated
@@ -23,6 +42,51 @@ const newConversationItem = {
}
};

const emojiSubmenu = [
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we should move this and other related logic to module.exports = {emoji} in emoji.js to contain all the logic for this feature in one file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was a good idea! Done. Let me know how do you like the new code organization.

@sindresorhus sindresorhus merged commit aa9c896 into sindresorhus:master Jan 18, 2019
sindresorhus added a commit that referenced this pull request Jan 18, 2019
Co-authored-by: Jarek Radosz <[email protected]>
Co-authored-by: Sindre Sorhus <[email protected]>
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.

3 participants