Skip to content

feat: auto inject Vimium C #225

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.

Sign up for GitHub

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

Jump to bottom
Merged
YunFeng0817 merged 4 commits into from
Jul 7, 2021
Merged

feat: auto inject Vimium C #225

YunFeng0817 merged 4 commits into from
Jul 7, 2021

Conversation

@gdh1995
Copy link
Contributor

@gdh1995 gdh1995 commented Jul 5, 2021 •
edited
Loading

describe the bug/feature 解决的问题或新增的功能

This commit tries injecting Vimium C to inner PDF Viewer, which is for #218 and gdh1995/vimium-c#383 .

summary of code change 描述发生的改变

It loads Vimium C's lib/injector.js on static/pdf/viewer.html loading, so that Vimium C will run on it.

It prefers the version of Vimium C on Edge Add-ons on MS Edge; while on Chrome it prefers the version on Chrome Web Store.

This commit only affects the PDF Viewer page, but not options page.
If there's a new text option to let users type an extension ID, then static/pdf/vimium-c-injector.js will use it.

@gdh1995
auto inject Vimium C for EdgeTranslate#218
c17dce2 
This commit tries injecting Vimium C to inner PDF Viewer.

It prefers the version of Vimium C on Edge Add-ons on MS Edge;
while on Chrome it prefers the version on Chrome Web Store.

This commit only affects the PDF Viewer page, but not options page.
If there's a new text option to let users type an extension ID,
then `static/pdf/vimium-c-injector.js` will use it.
@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 5, 2021

顺利的话，安装 EdgeTranslate 和 Vimium C，并打开 pdf 文件后，点击右上角的 Vimium C 图标将能看到：
image

点击“信任此扩展”后，当前页面应该会自动刷新，然后就能用 Vimium C 快捷键了。

添加了一个 vimiumExtensionInjector 的字符串配置项，默认自动根据浏览器是新版 Edge 还是其它来加载对应商店版本的 Vimium C。如果用户没安装的话就会有个 <script> 加载失败的报错。

所以可以改掉 vimium-c-injector.js 来跳过空值，或者在设置页面里加开关/文本框来动态启用这个。我不熟悉 EdgeTranslate 设置页面的设计，就不贸然修改了。

@YunFeng0817 YunFeng0817 requested review from nickyc975 and tboevil July 6, 2021 00:59
nickyc975
nickyc975 reviewed Jul 6, 2021
View reviewed changes
nickyc975
nickyc975 reviewed Jul 6, 2021
View reviewed changes
nickyc975
nickyc975 reviewed Jul 6, 2021
View reviewed changes
@gdh1995 gdh1995 changed the title auto inject Vimium C for #218 feat: auto inject Vimium C Jul 6, 2021
@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 6, 2021

呃抱歉昨晚上我没 wifi，我就没装 node_modules，没注意到 eslint 和 commitlint。新的提交按规则来了

YunFeng0817
YunFeng0817 reviewed Jul 6, 2021
View reviewed changes
@YunFeng0817
Copy link
Member

YunFeng0817 commented Jul 6, 2021

image
如果没有安装vimium-c的话，这里是不是可以麻烦再处理一下

@YunFeng0817 YunFeng0817 added enhancement New feature or request good first issue Good for newcomers labels Jul 6, 2021
@gdh1995
fix: try to connect to Vimium C before loading its script
a205023 
This avoids an error log, at the expense of a bit longer loading time.
@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 6, 2021 •
edited
Loading

改了。现在 vimiumExtensionInjector 如果是带 path 的 URL，就直接加载，如果是个 origin 或者 hostname，就先尝试发消息过去。不过格式是我自己编的，就不考虑另外的扩展了。

消息格式参考： https://github.com/gdh1995/vimium-c/blob/c81a225e636ead9595a81169192dbd277acd879c/background/main.ts#L158-L165

YunFeng0817
YunFeng0817 requested changes Jul 6, 2021
View reviewed changes
@YunFeng0817 YunFeng0817 requested a review from nickyc975 July 7, 2021 02:27
@YunFeng0817 YunFeng0817 merged commit 92869ae into EdgeTranslate:develop Jul 7, 2021
@gdh1995 gdh1995 deleted the inject-vimium-c branch July 7, 2021 06:16
@gdh1995 gdh1995 mentioned this pull request Jul 7, 2021
Closed
@gdh1995
Copy link
Contributor Author

gdh1995 commented Jul 28, 2021 •
edited
Loading

Hello, I find the hook window.VApi.$ in static/pdf/vimium-c-injector.js needs to be updated. How should I do this? Another PR, or I just put new code here and some of you maintainers apply it ?

The main places to fix are:

  1. The old VApi.$ may need more arguments and return some values, so replace oldScroll.call(...) with return oldScroll.apply(this, arguments);
  2. change the function from lambda to function (element, ... to make arguments avaliable.

@YunFeng0817
Copy link
Member

YunFeng0817 commented Jul 28, 2021

It would be perfect if you're willing to pull a request😀

@gdh1995 gdh1995 mentioned this pull request Jul 28, 2021
Merged
@gdh1995 gdh1995 mentioned this pull request Sep 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickyc975 nickyc975 nickyc975 approved these changes

@YunFeng0817 YunFeng0817 YunFeng0817 approved these changes

@tboevil tboevil Awaiting requested review from tboevil

Assignees

No one assigned

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gdh1995 @YunFeng0817 @nickyc975