-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fixes #6669: Allow images in deck description #8184
Conversation
First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible. |
Codecov Report
@@ Coverage Diff @@
## master #8184 +/- ##
=============================================
- Coverage 37.04% 15.44% -21.61%
+ Complexity 3776 1659 -2117
=============================================
Files 352 353 +1
Lines 35739 35776 +37
Branches 4732 4733 +1
=============================================
- Hits 13239 5525 -7714
- Misses 20992 29419 +8427
+ Partials 1508 832 -676 Continue to review full report at Codecov.
|
Hi Thanks a lot for helping improve ankidroid. Sadly, right now, unit tests fails. In particular,
Did you try running those unit test on your machine before submitting? The goal of commit title is that someone, in a few years, can look at history, and know easily what a commit do, and why a line of code was added. Sadly, #6699 is not descriptive.
In your screenshot, we see html on the bottom left, I don't expect that what we would like to see actually. Not sure what the ideal result is. I believe that no html would be good, and adding the html back when the user put the focus on it and start to edit. Probably worth doing an extra feature request for that. I'd like you if you can test with let's say 20 images and check whether it loads normally or slowly. I assume that it will load one image after the other here if no actualy background task is used |
mTv.setText(spanned); | ||
} | ||
|
||
@SuppressWarnings( "deprecation" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not. If we can't avoid using a deprecated method, at least I want us to know that we use deprecated method, so that we can correct later. Did you check whether actually there is nothing better than the dprecated one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sought to load images using bitmap, drawable and ImageGetter only and it was giving this warning with all of those during the build so I had no other choice.
import android.widget.ImageView; | ||
import android.widget.TextView; | ||
|
||
public class HtmlImageGetter extends Activity implements ImageGetter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I please ask for some javadoc.
I understand that this gets image from HTML, but I'd still love to understand why it's needed and implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find a definitive javadoc but here's android reference to it ImageGetter
d.addLevel(0, 0, empty); | ||
d.setBounds(0, 0, empty.getIntrinsicWidth(), empty.getIntrinsicHeight()); | ||
|
||
new LoadImage().execute(source, d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of directly executing an async task ? If you are not running it asynchronously, that seems useless.
I doubt async task is the proper tool anyway, I thought async task where made to run things one after the other, and we probably want to start all download at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You get android.os.NetworkOnMainThreadException
when it attempts to perform a networking operation on its main thread
The original bug report mentionned using webview. I don't know enough about it to know the difference, but maybe @david-allison-1 can check whether not using it would be a prolbem |
I will surely rephrase the PR. |
It seems like using a WebView may be the correct decision (depends on what Anki Desktop does), but maybe restrict it from internet access until we confirm a need for it. This is one that we should probably check with Anki Desktop's source code, and maybe dae about to ensure that he doesn't have concerns about the implementation.
|
@Ekagra Can you try this sample for testing if it is working or not? It is used for showing update available for decks. As user have to manually check for deck's updates. So, I have implemented simple script for doing that. It is working on AnkiDesktop but due to not having webview, it is not working in AnkiDroid. I have added js script to fetch deck version from GitHub and compare with deck version in deck description. If there are changes then it show update available. On desktop it shows update available for decks View versionPopup.html in deck description AnkiDesktop AnkiDroid So, what will be approach? |
Alright, so we only have to use webview to load images? You can update about what is the correct way to do it and how the desktop version does it. |
Yeah, I'll check it out and see what needs to be done. |
So, to summarize all the feedback:
|
Future versions of Anki will block JS from the deck descriptions - the fact that it's currently allowed was just an oversight. |
Thanks! Do you plan to continue to allow external content ( |
At the moment images are filtered out when the new code is enabled, but I suspect deck authors will ask for them again. Local images are likely not an issue; remote images are potentially a tracking bug - but we allow them on cards already, so special-casing the overview screen doesn't seem to buy us much. An option in the preferences to disable remote images everywhere might be nice at one point |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
I think we've been quite busy with 2.15 and just not had time to work through this one - unstale-ing for now |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
Pull Request template
Purpose / Description
Fixes #6669
Fixes
#6669
Approach
I've made a HtmlImageGetter class that displays the html image using drawable and ImageGetter, I've used ic_add_toolbar_icon as placeholder.
How Has This Been Tested?
Checklist
Please, go through these checks before submitting the PR.
if
statements)