Allow "file:" and "[[...]]" links, display images in notes #389
Conversation
Cool!
What's the expected size of the displayed image when it's too wide? I'm seeing half a screen width in portrait mode and the same size in landscape. Should it depend on orientation?
I don't think this is displayed in Org mode. Only images in square bracket links. I think we should do the same.
Yeah, we'll want these displayed too.
Sounds good. Perhaps even keeping the feature off by default, though I'm not sure about that. I'll leave few comments in the code too. Thanks! |
| if (file.exists() | ||
| && (file.extension.toLowerCase() == "png" | ||
| || file.extension.toLowerCase() == "jpg" | ||
| || file.extension.toLowerCase() == "bmp") |
There was a problem hiding this comment.
toLowerCase can be called multiple times here. I'd suggest moving those to a new method (isImageExtension or whatever). Also, I'd call that before file.exists(), as I think it might be faster. I'd even use path instead, to avoid creating File. This is one of the places which we should optimize if we can, as it can be called quite a few times fast (when scrolling for example).
Are those 3 extensions the only ones supported for Drawable BTW? What about jpeg for example?
There was a problem hiding this comment.
I've update this code, and the supported extensions via the ImageUtils class.
I did not find a portable way to know the supported image formats, so I used the ones described in the android documentation: https://developer.android.com/guide/topics/media/media-formats
| // Read the image in a Drawable | ||
| val currentActivity = App.getCurrentActivity() | ||
| if(currentActivity != null) { | ||
| val inputStream = currentActivity.contentResolver.openInputStream(contentUri) |
There was a problem hiding this comment.
Does this need closing? Probably not, but it might be cleaner if it's done?
| } else { | ||
| cs = configureFileLink(ssb, link, m.start(), m.end()) | ||
| } | ||
| } |
There was a problem hiding this comment.
This splitting and checking for number of elements is a bit hard to read. I was wondering what's == 1 for, but that's for links such as [[file.jpg]] ? So it's basically a link with file scheme or no scheme? Maybe using something like fun fileLinkPath(link: String): String? would be easier to read, also avoiding this last check too?
There was a problem hiding this comment.
If I remember well, the == 1 is because when splitting a string without the specified delimiter, the method will return an Array of size 1 with the original string, so it should handle the case of not having a file: scheme.
Indeed I'll try to simplify this !
|
Few more things... Displayed images are now clickable too. Do we want that behavior? I don't really have a preference. Images can now be included and displayed in note title too. I don't think we should support that, even though it seems kinda cool. It's messing up the layout which could be tricky to fix. And even when fixed, I think notes would be harder to read with variable height titles (multi-line or not). Then again, Org mode support that. Links like Code style needs some fixing. Space after |
|
Ok, I've updated the code a bit, but there are still missing points:
Here I am not very happy with the use of
I usually don't display my images in org-mode, so it did not bother me to have it displayed, but I can disable that if you prefer.
I haven't fixed this yet. It seems to behave differently than the unnamed links, and I was not able to find why as of now.
I've added a boolean in the Config object, this should make it easier to set off.
I think this is a good idea to keep this, in case the user wants to display it in an image viewer for zooming (for small images) or modifications, in the same way you can open external links in org-mode.
This should be fixed with the additional config parameter I added. I also took the liberty to remove it from the reminders, as It would probably also mess with the layout in narrow spaces.
I've modified the function generating a ClickableSpan, so it does not create a link when the link is not valid for a file. It kinds of implement indicating that the file is invalid, as we discussed in the previous PR.
The code style should be fixed hopefully ! |
I've been using 2560x1600 JPEG and seeing:
for whatever reason.
It's tricky because of possible indentation too. Higher-level notes will have less space available. Also, there could be text before and after the image on the same line, though that will wrap. This is what I'm seeing:
Which looks fine, but it's unexpected. I thought image would take more space. I haven't tested it with different density devices.
Perhaps adding some tests to
I'm more concerned about the speed of loading images now. BTW, I noticed |
|
Indeed, I haven't tested with notes on other levels, so the pebble/butterfly image should be exactly the same size on higher level notes. Regarding to the loading speed of the images, there seem to be some LruCache available on Android, see https://developer.android.com/topic/performance/graphics/cache-bitmap. I'll try to look at your other remarks when I have more time ! |
Yes, I agree. It's just that I don't understand why this huge image doesn't get to fill the whole width. So I'm worried that there could be issues on some other devices. But if we check few more different sizes and densities and it looks OK, I'm fine with that.
Notebook in the screenshot I posted might not be a good example of a typical usage. Most likely the images will be all different. I can easily see users having bunch of them in many notes. So, caching might not be as important as loading images in the background. Not that we shouldn't try to use cache if it's not too hard. If Glide can help in any way, great. We shouldn't write more code then we need to, I don't mind external dependency unless it's for something trivial. |
|
I've pushed another update, implementing image asynchronous loading and caching, which should hopefully improve performance. This was harder that I thought at the beginning ! Especially with the use of ImageSpan (It seems easier with classical views) I ended up not using glide, as it was not that easy to use, and they seem to be changing APIs right now, with the deprecation of some objects. I used an AsyncTask and the LruCache. One issue that I have now is with the preferences. When toggling the new ones I added, the settings don't get applied until a restart of the application. If you have any clue of what I might have done wrong, please tell me, so I can fix this. I also reworked the code with the I've also tested the app with images similar in size of the big ones you use, loading and scaling is much better for me now. |
|
I don't see any images now, just linkified Wouldn't this also have issues due to view recycling? And also potentially start bunch of I don't think this'll be that easy to get it right. We should definitely check how it's being done in some apps out there. It's a common thing to do, though I agree, perhaps not so much with
I think it's OK, but then I'd remove BTW, I don't see anything wrong with the preference - when I install app for the first time, it's set to |
Do you still use the same platforms as in the previous PRs ? Do you have the same issue on all the platforms ?
While scrolling fast I didn't encounter any issue on my side.
I'll look for similar stuff, but yeah it's less common with ImageSpan that classical views.
Ok, I'll go this way, once I sorted out the loading issues. Also regarding your previous comment:
I can also reproduce this, but it seems that |
Yes, I also tried to checkout the previous commit and I do see the images there. Then going back to latest one, I don't.
What are you seeing there? I haven't tried API 16.
I suspect there could be updates to the wrong views, but we'll see.
But this shouldn't pass the check for extension though? |
It's really very strange, because I've tested it on a fresh image with a Nexus 6P API 25 and it works as expected... I don't get where the issue is.
The issue here was with a bad test that broke the image display when the image is at the very end of the note and that the
There is an additional problem with views: When the storage permission is not granted at the start of the app, then launch the app, try to click on a link to make the permission snackbar appear, grant the permission, then back to the app, the image are not immediately refreshed. I suspect that the view are not refreshed when resuming the app.
The extension check is only performed when we want to display an image with an ImageSpan. I don't know if I will have much time this evening, but I'll do my best to sort all those issues ! |
Ah, it looks like this is it. All my notes (well, only 3 unique, repeated bunch of times) are like that. Images are displayed when I add a space after Few more things I noticed... Images stop being displayed when I:
When scrolling up slowly, there is sudden jump as note comes into view, I assume due to image being inserted and note now occupying more space. With scaling option on, images occupy maximum width, even if there is some text after them, which then wraps. Might be OK, I'm not sure. With scaling option off, aspect ratio doesn't seem to be kept. When toggling above option, only images from first few notes are displayed. When I keep scrolling down, there are no images. When I go back to the top, few images previously displayed are gone. |
This should be fixed in the latest commit !
I also updated this, so only the width is fixed. The height is computed to keep the aspect ratio.
I have a similar issue, when I toggle one of the settings ...
Yes, I agree it is a bit disturbing, and I don't really know how to compensate for that.
This is indeed debatable, and that's also why I put a scaling parameter to give the choice to the user. |
Cool. In both cases (scaling or not) - I don't think image should be scaled up. Or at least there should be another option to control that? Also, current hardcoded
Current (tracked) activity is lost after returning from Settings:
should be enough, removing everything from other methods. I don't know if this is also the reason for the issue you seeing - not being able to return to the notebook.
Without knowing image dimension in advance, it's probably not possible. User experience could be improved though:
|
|
I've fixed snackbar issue in master, hence the conflict, though it should be easy to resolve. I've also created OrgFormatterLinkTest so we can list all the cases - should the link be parsed or not, clickable or not etc. This test is failing right now, there are two fixes needed in |
|
Ok, thanks for the update ! Yes indeed I agree for the split. |
I prefer rebase, but whatever is easier for you. Thanks! |
With scaling on, images should never be scaled up. They are either kept to their original size if they fit within the view width or scaled down to fit the current view width.
I agree, this was mainly as a first test, I'll add a preference to set this.
Indeed, I remove the else part of the registerActivity test to implement what you propose
This doesn't seem to be fixed. I'll investigate this.
Great, I'll see what I can do on this side, although I'm just not sure that item animation is possible.
I can go for a rebase. I prefer asking because some people don't like forced pushes ! |
No, you're right. It due to different issue I encountered. I pushed a new tiny image to the emulator by overwriting the old one, left the list (or app), returned to it and went back to the book. I saw the same image, not the new one. It's due to caching by name probably and app didn't terminate. I don't think we need to keep the cache during entire app run. It's enough to create it when user enters the notebook (or search results). But even then, we'll need a way to invalidate the entires. And/or store them by something other then just file name. I'd much rather have cache misses here, then stale data. Cache is much less important now with async loading. I don't consider this a blocker for this PR though. Just moving the cache from
Is it #348 or similar perhaps? That would be helpful, as I wasn't able to reproduce that one.
There's probably a way (worst case generating two item views and animating the replacement, or something like that). But definitely not a blocker for this PR, we can do that later. |
aancel
force-pushed
the
update-external-file-image
branch
from
ef519d0 to
977b55e
Compare
|
Okay, rebase has been done on the latest commit. I've encountered 2 merge conflicts, I had a look at your commits, I hope to have done the things correctly with this rebase. Regarding to the permission snackbar, I am now having the same issue as I had before.
Regarding this kind of change of the image, maybe using a hash as the storage key instead of the path could solve the issue ? Adding the overhead of hash computation, but it would then be a reliable cache |
It indeed seems to be the same thing that is happening to me. The |
|
Regarding the broken UI actions, I might have found something, but not so sure. I report it here so maybe it will help. I focused on opening a notebook which works in the normal case, but not in the buggy case (meaning having toggled a button in the preference). I used the debugger and located an area that should be the cause of not opening the notebook. In the normal case, and going trough all the functions I end up in the LocalBroadcastManager on line 255: Here |
Ah yes, fix is only for snackbar called from
I don't know if it would be fast enough. You'd have to open and read entire stream, calculating the hash. Not to mention you'd have to do open and read the stream again if you get a miss, unless you kept the content the first time. Though that would be done asynchronous. It's worth testing I guess.
It might be something to do with activities being restarted on preference change. Try disabling |
|
I've taken screenshots of the UI while debugging this gives a hint at the values when the issue occurs: The case of notebook not opening: The action associated to the intent does not seem to be found in the bad case. In the ok case, mActions contains:
In the bad case, mActions contains:
|
Ah, nice find. Missing actions are registered in
|
|
Latest small update refactors the code a bit, ensure that we only register the lifecycle of CommonActivity and adds a setting to update the size of fixed-width images. I'll have a look at your comment on missing actions. I'll see if it solves the issue for me. |
I agree, It should definitely be a new PR and we can move any further conversation to #348. |
|
Ok for the removal of the milestone ! I'll keep on working on it as time permits (and I'll be happy with a new version too !) I did try rebasing the PR, but yeah, it's a bit more complicated this time. I'll see how it goes, but I might end up squashing commits to make it easier on my side. |
Thanks. It's definitely not trivial.
Yes, there needs to be a limited number of workers, depending on the number and size of images. We should really look into Glide, perhaps it's already solving this problem as well. It'll certainly help with some potential
There should be very little changes to existing code. I'm thinking a different approach might be better. Loading images could be done outside of |
|
I've given glide a try previously, but I did not seem to be working very well with ImageSpan. I can give it another try, but I'll first update the code base to the latest one. |
You don't really have to rebase, you can just continue the work and I'll merge and resolve this work later. You could just work towards the current code in master, to make resolving conflicts easier. Or I can modify
Yes, over |
|
Yes, I see what you mean, but it's just a way to make things easier on your side. Just a quick question I have regarding the setText method of the TextView. It's just to be sure about preventing issues with the async part of image loading (I didn't find the information as of now). |
It can be stored as
I think we could simply do:
Does that sound OK? Do you need anything else apart from |
|
Ok, thanks a lot for the input and explanations !
Great, it is fine for me. So if I understood well, this code would end up in the setRawText function of TextViewWithMarkup ? (unless you prefer to keep the opportunity to have images in reminders and titles)
Yes, that's OK. The FileLinkSpans will do fine. I'll have a look at this ! |
I don't think we modify only spans anywhere right now. I think in all cases text has to be changed (even for checkbox toggling, though that could be done differently).
I've made the changes for this in 813f4f9. Not exactly what I suggested, but kinda. You might want to make |
- Background load images with glide - Add new settings: - Enable image display - Set fixed size image or enable scaling - Set fixed size for images - Add function to process input paths and drawables
aancel
force-pushed
the
update-external-file-image
branch
from
e638efe to
1e6a95a
Compare
|
Ok, so I've rebased on your latest commit to benefit from the placeholder code you added. Hopefully, I didn't break anything, but eveything seems fine on API 16, 23 and 27. |
|
I am also keeping in mind the possibility to:
|
Great, thanks!
When I open the notebook (then go back, then open again, two times) I see:
If I start scrolling and go back, they all end up being the same (larger) size (which is still not large enough I think). |
Sure, both would be useful, new issue (if it needs more discussion) or PR would be great. |
| // Gather drawable information | ||
| // Scale the height by the scaledDensity to get original image size | ||
| val drawableHeight = drawable.intrinsicHeight.toFloat() * metrics.scaledDensity | ||
| val drawableWidth = drawable.intrinsicWidth.toFloat() * metrics.scaledDensity |
There was a problem hiding this comment.
Actually, why is this multiplied at all?
I just tried displaying a small icon (96x96), removing any other code below this, and it becomes a huge, low-quality image.
There was a problem hiding this comment.
Yes, my bad.
There a few things wrong with this code indeed. The metrics.scalingDensity artifically scales the small images. The width of the view is equal to zero. The size of the Drawable are not correct regarding to the original image size (I found a way to get the original size with BitmapDrawable)
I'll rework this code
| // And scaled thumbnails for faster display | ||
| .thumbnail(Glide.with(context) | ||
| .applyDefaultRequestOptions(RequestOptions().override(AppPreferences.setImageFixedWidth(context))) | ||
| .load(contentUri)) |
There was a problem hiding this comment.
How can this improve display? It's the same image, no? I think this is meant to be for actual smaller images. It's likely we are now loading two same images twice (unless Glide is detecting it's the same uri and refusing to do so).
There was a problem hiding this comment.
That's a very good point indeed.
Apparently and according to the documentation the thumbnail is loaded in parallel while the main image is loading, so it should indeed load the image twice.
This was part of a test that I did. I noticed that without the thumbnail code I ended up with around 80 mb of memory used (via the visual profiler) on my sample test, and with the thumbnail code, I ended up around 38 mb.
Can you confirm this behavior ?
I would guess that the main image is not loaded and only the downsampled thumbnail. I'll dig deeper to figure out what happens behind the scene. There might be a way to save memory here.
There was a problem hiding this comment.
Sounds like it could be the reason for random size images then? I think we should just drop thumbnail().
|
|
||
| // Setup a placeholder | ||
| val drawable = ColorDrawable(Color.TRANSPARENT) | ||
| drawable.setBounds(0, 0, AppPreferences.setImageFixedWidth(context), AppPreferences.setImageFixedWidth(context)) |
There was a problem hiding this comment.
I still get the jump for the fixed size images. Looks like this is setting the height to the width, not calculating the ratio like for the actual image display.
I also noted this behavior, it seems that the textview is not refreshed properly when changing the settings, and it does not seem to happen all the times.
Ok, I will open a PR then ! |
- Switch to Bitmap/BitmapDrawable to get native image sizes - Resize the bitmaps with Glide by pre-reading image size
|
(Sorry this post was written a bit in a hurry)
Let me know how it works for you |
|
I've fixed few things - preferences' dependencies, images were still being scaled up, placeholder was not being set at all. There are still some issues - images are resized twice, ratio is lost on second resize in some cases, storage permission requirement needs to be made more clear, etc. |
|
Thank you for the merge and thanks a lot for the fixes/testing/advices you gave on this MR ! |
|
I am trying to find out this weeks ago: |
Files need to be inside primary storage directory ( It doesn't look like Can you open a new issue so we can track this and see if there are alternatives? Thanks. |
|
Like this? |
Hello,
Following the work on #382, I open a PR with new propositions.
I reworked the code a bit to better integrate it with the previous code, and notably to handle
[[...]]for file links (still linked to #360) in addition to the previousfile:scheme.I also added an ImageSpan to display images in notes (rescaled to fit width, if larger than the window width), as a proposition to implement #147.
As before, this PR has been tested with API 16 (Nexus 5), API 23 (Pixel 2) & API 27 (Pixel 2 XL)
A small note sample, I used to test this:
Right now, named links are not replaced with an ImageSpan.
Furthemore regarding to the application performance, maybe adding a setting to toggle off image display could be a good idea. What do you think about this ?