search

arianneorpilla / jidoujisho

A full-featured immersion language learning suite for mobile.
GNU General Public License v3.0
912 stars 58 forks source link

Fixed adding of images and audio from immersion kit #172

Closed okkuweb closed 1 year ago

okkuweb commented 1 year ago

More of a note of a regression in the immersion kit functionality. Adding of images and audio from immersion kit suddenly stopped working on the newest version so I traced down the issue. The code that adds the images and audio to the notes was moved from the onAppend event to onSelect event. Since there doesn't seem to be support for multiple images (?) at least I thought that it would be fine to move the code back for now.

Not sure if there was some idea behind the change, but since I use this feature I thought I might as well make pull request for it just in case. Feel free to reject if I missed something.

arianneorpilla commented 1 year ago

Hi @okkuweb. The idea is that in the other sentence enhancements, append adds example sentences instead of setting them.

This implies that there is already a primary sentence prior to the to-be added sentences. Ideally, the first sentence that the user adds is the one they will read first, so it is ideal that the media is related to the first sentence. Since appended sentences are considered secondary and are not the priority, the new behaviour that I went for is to only add text on append.

It has been suggested before that I separate word image from sentence image, or that I support multiple images, and it is something that I am thinking about, but I feel that it will complicate the UX to have multiple images/pickers and I don't feel that users really need more than one image on their Anki card -- I feel like I should promote simplicity in that regard.

arianneorpilla commented 1 year ago

I hope that explanation will suffice. I've been for the most part alone with this so I am really happy to receive your PR regardless -- I'm glad people are using this thing and care enough about it as their workflow in regards to the changes I'm making.

If you can perhaps explain a unique use case that my change does not consider or if you can suggest an alternative behaviour, I am all ears.

I am currently on hiatus and the on select behaviour works fine as I intend and someone such as yourself can edit the source code as befits them so I don't think this is time sensitive, but I will keep things in mind when I get back to it.

okkuweb commented 1 year ago

I totally missed that select button for some reason. I'm so used to the rightmost button being the "cancel" button that I didn't figure out that there's two buttons for selecting/appending a sentence.

I've been following the project for a while and since I noticed that immersion kit support was added at some point I gave it a whirl. Probably gonna replace my current dictionary with it now. Just had this one minor hickup. I have no previous android/flutter experience so I dunno how much help I can be, but I'll keep using the app so I'll see if there's anything I can help with at some point.

okkuweb commented 1 year ago

I'll close the request since I don't have anymore issues.