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

feat: storing thumbnails on disk #201

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

Rentib
Copy link

@Rentib Rentib commented Oct 18, 2024

Closes #200

@Rentib Rentib marked this pull request as ready for review October 18, 2024 14:55
Copy link
Owner

@artemsen artemsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this feature should be optional. We need an option in the config file that will allow saving thumbnails to disk. Writing anything to persistent storage by default is not a good idea.

src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/pixmap.c Outdated
return false;
}

// TODO: add alpha channel
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 14 supported image formats. It would be better to use one of them instead of reinventing own format.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any function for writing an image to disk in the codebase. Did I miss something?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current code doesn't contain code for encoders. But these APIs are available to swayimg.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to keep current ppm format or should I change it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to add encode_pnm to PNM(PPM) decoder.

@Rentib Rentib force-pushed the cache_thumbnails_on_disk branch 4 times, most recently from 8f74942 to 25fa0e2 Compare October 19, 2024 13:06
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/pixmap.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/image.c Outdated Show resolved Hide resolved
src/pixmap.c Outdated Show resolved Hide resolved
src/pixmap.c Outdated Show resolved Hide resolved
@artemsen
Copy link
Owner

artemsen commented Oct 23, 2024

Please, rebase to latest master. This will fix Ubuntu CI (but this doesn't resolve the warnings you have =).

src/pixmap.c Outdated Show resolved Hide resolved
src/thumbnail.c Outdated Show resolved Hide resolved
src/thumbnail.c Outdated Show resolved Hide resolved
src/thumbnail.c Outdated Show resolved Hide resolved
return true;
}

static bool get_thumb_path(char** path, const char* source)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static chat* get_thumb_path(const char* source) would be clearer.
Bu the way, source can contain prefix exec:// or starts with ../../../../../../etc/passwd, so which path you finally create?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is a problem. I will fix it to not pass source, but the absolute file path.

src/thumbnail.h Show resolved Hide resolved
src/thumbnail.c Show resolved Hide resolved
src/thumbnail.c Show resolved Hide resolved
struct thumbnail_params params;

/* TODO: move to config */
bool thumbnails_disk_cache = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be a part of the static context for the thumbnail entity.

src/thumbnail.c Show resolved Hide resolved
@Rentib Rentib force-pushed the cache_thumbnails_on_disk branch 3 times, most recently from 2ef0a74 to 88f2d10 Compare October 27, 2024 23:55
refactor: format code

Signed-off-by: Rentib <[email protected]>
breaking change: image_thumbnail() takes arguments

Signed-off-by: Rentib <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: storing thumbnails on disk
2 participants