Skip to content
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

Size and quality options for Anki screenshots. #143

Open
cessen opened this issue Jan 18, 2023 · 8 comments
Open

Size and quality options for Anki screenshots. #143

cessen opened this issue Jan 18, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@cessen
Copy link

cessen commented Jan 18, 2023

(Since #19 was closed due to the original requester deciding they didn't want it after all, I'm opening a new issue.)

If possible, it would be nice to have size (resolution) and compression quality options for screenshots saved for Anki notes. This isn't for display purposes. I'm aware that the visual size can be controlled with CSS in Anki, which I already do. Rather, this is for the on-disk size.

Further, I'm not concerned about the on-disk size on my local computer(s). Disk space is cheap. Rather, I don't want to take too much advantage of AnkiWeb's free sync service, especially as my deck size grows. And it also affects sync time between devices. So I would prefer to be able to make the screen shots as small and compressed as I'm comfortable with. For me, they just serve as a reminder of context, and don't need to be high res or high quality.

(A quality option for audio might also be useful, but audio files are pretty small to begin with, so probably not worth it.)

@ripose-jp
Copy link
Owner

I don't want to take too much advantage of AnkiWeb's free sync service

This was actually the driving factor behind making JPEG the default encoding instead of PNG (WebP didn't make the cut due to compatibility). I respect that dae doesn't accept donations, so I don't want to put any more strain on his service than I have to. That said, I still have problems with this feature.

My concern about complexity in the settings is unchanged. Anki Integration Settings have only gotten more complex since that issue was made, even if most of it is hidden by default now. Frankly I think Memento's settings design is atrocious in general, but I don't have any great ideas on how to make it better.

I also wonder if image resizing is something Memento even needs to implement considering https://ankiweb.net/shared/info/1214357311 exists. I haven't used it so I can't vouch for how well it works, but it looks like it does exactly what you want.

Something I would be open to adding is support for newer, more efficient formats. My issue stems from the fact the only two decent options are AVIF and JPEG XL. AVIF has long encoding times and can easily force a CPU to go full throttle for tens of seconds. JPEG XL is better in everyday, but someone at Google has decided to remove support for it in Chromium, so it will likely never make it to Anki in spite of being the superior option. That said, I don't have any qualms about adding AVIF support as an option for people that can overlook its problems. I just won't make it the default unless encoders get much better.

@ripose-jp ripose-jp added the enhancement New feature or request label Jan 18, 2023
@cessen
Copy link
Author

cessen commented Jan 18, 2023

Is your complexity concern primarily about the preferences UI getting cluttered and hard to understand for users? Or is it something else?

I was imagining this would be a config-file only option. E.g. with a memento.conf alongside mpv.conf in the config directory. If you're open to that, I'd be happy to work on a PR (when I have the time... not sure when that will be).

@ripose-jp
Copy link
Owner

Yes I'm worried about the UI becoming overwhelming. I don't want users to need a guide to know how to navigate my software.

I was imagining this would be a config-file only option.

I'm not opposed to this at all, but the way Memento currently handles config files is a bit of a mess. Most settings in Memento are handled by the QSettings object. I've left it with its defaults, so settings are written in different places on each platform. On Windows settings are in the registry, macOS they're in a plist file in the Library folder, and on Linux they're in ~/.config/memento/memento.conf. I can change this as QSettings has an option to set the format to be standard across platforms. If I were to do this, code would have to be written to migrate settings from the old format to the new format.

To complicate things Anki Integration Settings are stored entirely independently of all other settings in a file called anki_connect.json. The reason for this is Anki Integration Settings were the first persistent settings in Memento, so due to my inexperience I didn't use QSettings. Instead rolled my own format using JSON, which made sense at the time because AnkiConnect's API communicates with JSON. This would be the settings file that concerns this feature.

The next problem is that Memento doesn't sanitize inputs from settings files. Since they were never meant to be edited by hand, all the validation is done on the front end at the time they are entered. It's almost certainly possible to trigger undefined behavior in Memento by inserting invalid values in the right options. I didn't consider this a concern in the design of Memento because settings were never meant to be edited by hand to begin with. If settings were to be edited by hand and the result was Memento crashing, the user wouldn't have anyone to blame for that but themself.

The last problem is the simplest. If I were to allow settings to be edited by hand, I'd just need to start documenting everything. Probably in a wiki page on the repo.

All this is to say that going about implementing this feature through config files is a much bigger project than it seems. If that's something you're up for by all means make a PR.

@cessen
Copy link
Author

cessen commented Jan 19, 2023

I'm not opposed to this at all, but the way Memento currently handles config files is a bit of a mess.

My first thought is that since at the moment the anki-related settings are in anki_connect.json, I could just add these settings there for the time being since at least that wouldn't make the situation worse. And then a config file rework could be done in the future.

Having said that, getting the config file situation cleaned up is also something I'd like to take on. At least, in theory. In practice I have my own work and open source commitments, so I don't have a ton of spare time/energy to dedicate to this. And I don't know when I'd be able to get started. But one of the things I love about vanilla MPV is that its settings are all config-file based, which makes it easy to keep my preferences synced across computers by versioning my dot files with git. So making all the settings stored and accessible that way is definitely something I'd be interested in helping to make happen, if/when I can find the time.

@ripose-jp
Copy link
Owner

I'm not big on adding hidden config options to anki_connect.json because I'm afraid it might encourage users to edit other settings outside the UI. Plus it just makes the problem of migrating from having two separate configuration files a little harder. While the settings tracked by QSettings can be migrated pretty trivially, Anki settings are going to need a lot more care.

The only two paths forward for this feature in my mind are either fixing the settings design to be more accommodating to complexity and adding this option on the frontend, or reworking configuration files to be edited manually. I'm not interested in stacking tech debt at this point in Memento's development without good reason.

I understand that you're busy and don't have much time to dedicate to this. Likewise Memento does everything I've ever wanted it to do at this point, so very few feature requests excite me enough to work on them. I can't make any promises on when I get to this, but you're welcome to maintain your own patch set for Memento in the meantime. I know this change can be done in a few lines if you want to hardcode a resolution.

@cessen
Copy link
Author

cessen commented Jan 21, 2023

I'm not interested in stacking tech debt at this point in Memento's development without good reason.

That's totally fair! I'll treat these as linked goals, then.

but you're welcome to maintain your own patch set for Memento in the meantime.

I might do that as a stop-gap. But that also sparked another stop-gap idea: for now, Memento could just always scale the images to a more Anki-reasonable size. For example, if someone's watching a 4k video, they almost certainly don't need a 4k screen shot for their Anki card. Scaling all screenshots down to a max width/height of 1024 or even 512 by default probably makes sense.

That still wouldn't be as small as I personally want, and it doesn't address the quality setting, but at least it would avoid people accidentally loading Anki's servers full of high res screen shots.

Would you accept a PR along those lines in the meantime?

@ripose-jp
Copy link
Owner

Would you accept a PR along those lines in the meantime?

I don't want to change the behavior of screenshots since there are bound to be people that like things as a they are.

Your suggestion did give me an idea that would make both of us happy though. Rather than adding new settings that affect all screenshots, we can extend marker syntax. This would allow each screenshot to be scaled independently of each other and doesn't require refactoring configuration options or the frontend. Something simple like {screenshot:max-width=1280,max-height=720} would suffice. I think this idea has the potential to greatly simplify the frontend by moving options from there to this new syntax. For now though, just working on getting {screenshot} and {screenshot-video} to work is fine.

Your thoughts?

@cessen
Copy link
Author

cessen commented Jan 24, 2023

That would definitely work for me, if you're happy to do things that way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants