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

Support for Safari #53

Merged
merged 1 commit into from Jan 19, 2021
Merged

Support for Safari #53

merged 1 commit into from Jan 19, 2021

Conversation

ken-matsui
Copy link
Contributor

@ken-matsui ken-matsui commented Dec 29, 2020

This PR brings support of Safari by using safari-web-extension-converter. If you have macOS PC, you can release it on App Store; also, this article might be useful for you if you are highly motivated to do that. I would appreciate it if you could do that. If you do not want because of Apple Developer Program, I would like to install this as a Safari Web Extension personally, and I think README.md should be updated.

Moreover, one thing that you should know is that Safari has less storage than the other browsers even though you specified unlimitedStorage in manifest.json. Apple states the local storage limit is 5 MB, and unlimited storage permission increases limit to 10 MB (ref). However, this storage is so few that large-size dictionaries like EIJIRO-1448 could not have been loaded due to storage quota exceed.

regarding to: #25

@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #53 (0754816) into master (e596dd4) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   98.75%   98.77%   +0.01%     
==========================================
  Files          21       21              
  Lines         883      897      +14     
  Branches       26       26              
==========================================
+ Hits          872      886      +14     
  Misses          8        8              
  Partials        3        3              
Impacted Files Coverage Δ
src/main/core/entry/en.js 99.46% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e596dd4...0754816. Read the comment docs.

@wtetsu
Copy link
Owner

wtetsu commented Dec 29, 2020

Aside from lack of my current motivation to publish it on App Store, supporting Safari build sounds like a great idea. Let me try the Safari build soon on my MacBook.
By the way, could you please share "this article" again? The link looks broken.

@wtetsu wtetsu self-assigned this Dec 29, 2020
@ken-matsui
Copy link
Contributor Author

@wtetsu
Thank you for your response, and I am sorry for sending you a broken link. The following link is what I wanted to send you.

https://developer.apple.com/safari/extensions

@wtetsu
Copy link
Owner

wtetsu commented Jan 2, 2021

@ken-matsui

Hi, were you able to build the Actions' artifact dist-safari, and installed Mouse Dictionary on your Safari?

https://github.com/wtetsu/mouse-dictionary/actions/runs/450896512

I faced two types of errors.

Error 1

error: ~/dist-safari-tmp/options: No such file or directory

To place mouse-dictionary build directory on my home directory resolved it,
but I think this workaround is strange and not reasonable for normal developers.

Error 2

Unknown attribute 'main'

Probably, this is due to my old Xcode.


  • macOS 10.13
  • Xcode 10.2.1

@ken-matsui
Copy link
Contributor Author

ken-matsui commented Jan 18, 2021

@wtetsu
I apologize for replying late.

Error 1

I also got the same error with Error 1. To build by safari-web-extension-converter, I had to build to Chrome Extension (it is dist-safari-tmp) and then convert it using safari-web-extension-converter (it will be placed in dist-safari). However, I did not know dist-safari refers dist-safari-tmp, and this is the reason for Error 1. Therefore, I pushed a new commit to fix the problem. Could you please check it again?

I could build the latest artifact and install built Mouse Dictionary on Safari.

Error 2

I did not see the same error, so it seems your Xcode is old. For your information, the following is my environment versions.

  • macOS 11.1
  • Xcode 12.3

@wtetsu
Copy link
Owner

wtetsu commented Jan 19, 2021

@ken-matsui

Thank you for resolving the issues.

1 -> I made sure it has been resolved
2 -> I succeeded to build it using @NSApplicationMain

Due to personal reasons, I cannot update macOS and Xcode,
but I made sure builds succeeded so I'll merge this Safari support as an experimental feature.


Before merging it, would you please delete .DS_Store?
(Please remove it from the history and force-push it)

@ken-matsui
Copy link
Contributor Author

@wtetsu

Thank you so much for your review. I removed .DS_Store and force-pushed it.

@wtetsu wtetsu merged commit 9f1d144 into wtetsu:master Jan 19, 2021
wtetsu added a commit that referenced this pull request Jan 19, 2021
@wtetsu
Copy link
Owner

wtetsu commented Jan 19, 2021

Thank you!
I've merge and added some information README.

@ken-matsui ken-matsui deleted the support-for-safari branch March 9, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants