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 link sharing functionality (#103) #127

Closed
wants to merge 1 commit into from
Closed

Added link sharing functionality (#103) #127

wants to merge 1 commit into from

Conversation

tswistak
Copy link

@tswistak tswistak commented Aug 14, 2021

Hi,

I've decided to add "Share link" functionality, raised in issue #103. I know there is already a possibility to share any link from the entry by long-pressing it. Still, this functionality is not very obvious (I've learned about it from the code even though I was using the app for some time), and I thought a separate "Share" button could be useful.

The implementation is based on downloading the dictionary entry in the background and searching for an anchor tag with id="view-online-link". All dictionaries made out of MediaWiki have an original link with such id. From what I've seen in the dictionaries list, most of them are from MediaWiki pages, and those that aren't, don't contain any other reference to the original. Of course, if, with some time, slob files would keep a reference to the original resource, it can be rewritten, but from what I've found out while using the Python's slob tool, dictionaries don't keep such metadata for entries.

I hope you like it, and if you have any comments on the code, I can fix it (I haven't programmed in Java for years, so I could do something wrong).

@itkach
Copy link
Owner

itkach commented Aug 17, 2021

I agree that long-press to share a link is difficult to discover and most people probably don't know about it. But clicking on a link (external ones are clearly marked same way as in Wikipedia) and sharing from a browser is fairly straightforward. So this feature doesn't seem to be a must-have.

The proposed implementation is problematic, mainly because parsing html with regular expressions is problematic in general, even if you're not looking to parse html fully. The regex is likely incorrect and doesn't work in all cases and it is likely to cause unexpected performance issues. Other issues include launching multiple instances of an expensive background task in a callback that may be called frequently (depending on user activity), reloading potentially large content, recompling regex on every call, assuming content is utf8 (not necessarily), assuming it is html (not necessarily), using printStackTrace() while the rest of the code base does not...

A better approach would be to query the document loaded in WebView instance using querySelector() from injected javasript that would call back into Java code. Another approach could be constructing external URL using dictionary's source tag and title. The first one is fairly complex, and the second one is not guaranteed to produce correct link though. Not worth the effort, IMO.

@tswistak
Copy link
Author

Fine, I understand. If it's not needed feel free to close the PR, I won't be mad about it, it still was a nice challenge to do this feature 😄 .
About the solution, I was thinking also about the second approach with JS, but unfortunately evaluateJavascript needs SDK 19 (project's minsdk is 15). That's why I decided to go with fetching HTML and Regex, since there is no problem with using them on older Androids.

@tswistak tswistak closed this Jan 10, 2022
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.

2 participants