Dictionary images #446

toasted-nutbread · 2020-04-11T18:41:11Z

Adds support for dictionaries with images. Resolves #322.

The following parameters are supported for images:

"path" - Path to the image file in the archive.
"width" - Preferred width of the image.
"height" - Preferred width of the image.
"title" - Hover text for the image.
"description" - Description of the image.
"pixelated" - Whether or not the image should appear pixelated at sizes larger than the image's native resolution.

As a simple test, create a dictionary using valid-dictionary1 data and scan 画像.

siikamiika · 2020-04-14T21:37:08Z

ext/mixed/css/display.css

+:root[data-compact-glossaries=true] .term-glossary-image-container {
+    display: none;
+}


Maybe they could pop out on mouse hover and show a thumbnail otherwise when compact glossaries is enabled

So there are two ways I could go about doing this. I am trying to do it without adding any additional Javascript, because that runs into the issue of adding/removing event listeners, which I would like to put off until it becomes necessary. (It may become necessary for some UI design changes I would like to make, but not yet.)

Small, matching the width of the "[Image]" text.

Large, matching the width of the <li> element.

Let me know which is better.

Assuming it's possible to use the :hover selector, I think it would be optimal if the element was visible when mouse is over the [Image] link. It could work similar to termTags in merge mode. If possible, I think the position could be in the center (position: fixed;) of the popup and the size limit could be something almost the width and height of the viewport. I think pointer-events: none; could help when mouse cursor is above the image but I didn't test.

Assuming none of the above is possible without some JS, I think the tiny thumbnail option is the best.

Right, I should have clarified that the images I posted are using mouse hover, and the mouse is hovered over the link in that case. So both of the two are possible using :hover.

Ok. Unless you think that it's bad UX to make the hover image possibly overlap with the link, I think fixed position would be the most practical at least inside popups (but maybe not the search page). It would also be possible to display a small image inline but that could be undesirable for those who want it to be really compact. As long as it's possible to change the behavior with custom CSS we should have everyone covered, though.

I currently went with the small image positioned relative to the [Image] link. IMO the large versions feel intrusive, and the small version fits more with the idea of being "compact". There are also positioning/sizing issues if trying to do the position: fixed version, primarily due to trying to deal with aspect ratio in CSS. So this is probably the most simple method until we get user feedback about it.

07e5e5c

Looks good 👍 My main issue was having to leave the entire page to even get an idea what the image is about. This fixes that part, but potentially makes the user do an additional step when the popup size would have been enough to understand the image contents completely. I didn't realize that there would be issues with aspect ratio and I agree we should wait for feedback before putting big effort into making it work.

I noticed that the small image will stay open when you click the link or the image and close the image's tab afterwards in Chrome. This could be a Chrome issue or a CSS issue. I didn't test in Firefox because the database upgrade would cause issues 😄

I noticed that the small image will stay open when you click the link or the image and close the image's tab afterwards in Chrome. This could be a Chrome issue or a CSS issue. I didn't test in Firefox because the database upgrade would cause issues 😄

This is due to :focus, which I added so that you can tab to the link to see the preview.

Ok, I think it's good to have it then.

siikamiika · 2020-04-19T02:35:54Z

ext/bg/js/dictionary-importer.js

+            mediaType,
+            width,
+            height,
+            source


Is there a reason why this is called source? Maybe base64Data instead.

I thought about "data", but it seemed redundant with "mediaData" and also non-specific as to what the data was. "base64" seemed superfluous to the name in the same way that doing something like "nameUTF8" would be. I chose "source" probably due to "source file" or "source code", but I agree "source" is also not fully specific (curse you English). Maybe "content", since it's similar to how it's used in the context of HTTP? (Content-Type, etc.) I suppose there's then a slight disconnect between using "mediaType" instead of "contentType", but that's a whole separate story.

I brought this up because I couldn't immediately see where the image data was saved to the database before tracing the code back to the line with const source = await file.async('base64');. For some time I actually thought that everything is done with the filename only but it felt strange to keep a file handle open to the zip that could be moved at any time.

"content" feels more intuitive to me than "source". "source" might work things like the original image path or dictionary name/ID. (But that's my non-native programming biased English speaker opinion. I've also been surprised many times when hearing a video game term used in the real world differently.)

I agree that base64* is not optimal, but at least it's something recognized by (web) developers. It's also used loadImage here: https://github.com/FooSoft/yomichan/pull/446/files#diff-50b9f3dfe5b96d91853bf0df8be101b3R55

Updated several things to improve API clarity. 7faaf4e...a7e7d54

However, this has made me notice one thing. This system currently loads images on the backend to validate them, but I don't think this will work with service workers. Not a dealbreaker, it just means that some of the validation will either have to be removed or moved into the settings page during import. There may also be other service worker issues with dictionary import due to how long the operation takes, but those are still unknown. #455

Updated several things to improve API clarity. 7faaf4e...a7e7d54

Thanks! I like the usage of base64 in the function name.

However, this has made me notice one thing. This system currently loads images on the backend to validate them, but I don't think this will work with service workers. Not a dealbreaker, it just means that some of the validation will either have to be removed or moved into the settings page during import. There may also be other service worker issues with dictionary import due to how long the operation takes, but those are still unknown. #455

I hope the move to manifest v3 isn't going to be a total disaster breaking everything like when Firefox moved from XUL to WebExtensions. At least XUL was making the browser development harder. I guess the background page thing was also a hack in lack of better APIs.

siikamiika · 2020-04-19T14:49:58Z

ext/mixed/js/media-loader.js

+    async _getMediaData(path, dictionaryName, cachedData) {
+        const token = this._token;
+        const data = (await apiGetMedia([{path, dictionaryName}]))[0];
+        if (token === this._token && data !== null) {
+            const sourceArrayBuffer = this._base64ToArrayBuffer(data.content);
+            const blob = new Blob([sourceArrayBuffer], {type: data.mediaType});
+            const url = URL.createObjectURL(blob);
+            cachedData.data = data;
+            cachedData.url = url;
+        }
+        return cachedData;
+    }
+
+    _base64ToArrayBuffer(content) {
+        const binarySource = window.atob(content);
+        const length = binarySource.length;
+        const array = new Uint8Array(length);
+        for (let i = 0; i < length; ++i) {
+            array[i] = binarySource.charCodeAt(i);
+        }
+        return array.buffer;
+    }


sourceArrayBuffer and binarySource could be changed to contentArrayBuffer and binaryContent for consistency. It seems that the word "source" was originally used in tests which also still uses "source".

Fixed the ones that are in the scope of this change.

toasted-nutbread mentioned this pull request Apr 11, 2020

[Feature Request] Custom dictionary with images #322

Closed

siikamiika reviewed Apr 14, 2020

View reviewed changes

toasted-nutbread added 6 commits April 18, 2020 21:31

Add support for importing and storing media files

8106f47

Add API for getting media data

fd6ea0e

Add support for displaying images

ac603d5

Add tests

5f49f0f

Fix space being inside the <a> and thus being underlined

c75a0d4

Show small image preview on [Image] link hover/focus

07e5e5c

toasted-nutbread force-pushed the dictionary-images branch from 2910f46 to 07e5e5c Compare April 19, 2020 01:33

siikamiika reviewed Apr 19, 2020

View reviewed changes

toasted-nutbread added 4 commits April 19, 2020 10:16

Use 'content' instead of 'source' to contain media file data

7faaf4e

Make getFileNameExtension properly handle directory separators

16893b5

Rename loadImage to loadImageBase64 for clarity

0e80c0d

Add documentation

a7e7d54

toasted-nutbread mentioned this pull request Apr 19, 2020

WebExtension manifest v3 #455

Closed

siikamiika reviewed Apr 19, 2020

View reviewed changes

toasted-nutbread added 2 commits April 19, 2020 10:57

Change some more variables using 'source' instead of 'content'

ba68616

Change some test variables using 'source' instead of 'content'

99c1a6a

siikamiika approved these changes Apr 19, 2020

View reviewed changes

toasted-nutbread merged commit a6344f6 into FooSoft:master Apr 19, 2020

toasted-nutbread mentioned this pull request Apr 19, 2020

Changelog #376

Closed

toasted-nutbread mentioned this pull request Aug 31, 2020

Settings dictionary import refactor #759

Merged

mathewthe2 mentioned this pull request Oct 18, 2022

Show images in dictionary arianneorpilla/jidoujisho#129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dictionary images #446

Dictionary images #446

toasted-nutbread commented Apr 11, 2020

siikamiika Apr 14, 2020

toasted-nutbread Apr 17, 2020

siikamiika Apr 18, 2020 •

edited

Loading

toasted-nutbread Apr 18, 2020

siikamiika Apr 18, 2020

toasted-nutbread Apr 19, 2020

siikamiika Apr 19, 2020

toasted-nutbread Apr 19, 2020

siikamiika Apr 19, 2020

siikamiika Apr 19, 2020

toasted-nutbread Apr 19, 2020

siikamiika Apr 19, 2020

toasted-nutbread Apr 19, 2020

siikamiika Apr 19, 2020

siikamiika Apr 19, 2020

toasted-nutbread Apr 19, 2020

Dictionary images #446

Dictionary images #446

Conversation

toasted-nutbread commented Apr 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siikamiika Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siikamiika Apr 18, 2020 •

edited

Loading