KamWithK / AnkiconnectAndroid

Unofficial Ankiconnect for Android (i.e. for Yomichan)
GNU General Public License v3.0
98 stars 14 forks source link

Merge media logic of addNote + updateNoteFields #41

Closed Aquafina-water-bottle closed 1 year ago

Aquafina-water-bottle commented 1 year ago

For some reason, when reviewing #31 and #28, I didn't realize that the same media-handling logic was pretty much implemented in two separate ways. A future version of ZXY101's mokuro fork requires updateNoteFields and url to work, and merging the logic of the two would be a simple way to support this.

@sp3ctum @Twinov As of writing this issue, I'm currently working on doing exactly this. Would y'all be interested in reviewing the PR whenever it comes?

Twinov commented 1 year ago

Sure. And yeah I think I remember looking at #28 and thinking that it would be good to extend the url logic to #31, I just didn't need it so I didn't bother :p

KamWithK commented 1 year ago

A little code duplication is okay given that it sometimes allows for quicker initial development, which is overall pretty valuable given most open source devs here work on a number of projects at the same time Cleaning the code up a little though is always a good idea though

mikavilpas commented 1 year ago

Sure, I'll follow this thread in case you'd like to comment here. Feel free to highlight me in case it suits you better, too.

Aquafina-water-bottle commented 1 year ago

Quick update: it seems like the fork is no longer using url and is using data, and this is already supported by updateNoteFields, so this issue is no longer very high priority for me. WIP / untested code can be found here for the time being: #43

If y'all are interested in continuing it, feel free to make another PR based off of it. Otherwise, I'll ping y'all sometime in the future when I'm actually done testing it / made sure the code style mostly matches / add comments / etc.

KamWithK commented 1 year ago

I'm going to close this issue then @Aquafina-water-bottle If anyone else wants to work on this just comment and I will reopen it up