Add full-screen ImageViewer widget. Support storing original files and thumbnails in the MediaCache

[aaravlu]

The difference between this pr and #341 is: when a image does not possess a thumbnail uri, this pr would put Original & 400x400 into image viewer while #341 would put Original & File into image viewer.

Note the change has nothing to do with timeline, only image viewer.

[kevinaboos]

Also, when you submit a PR, please give it a good, descriptive title and description. What i'd like to see different for this PR specifically:

1. A full title, such as "Add full-screen ImageViewer widget. Support storing original files and thumbnails in the MediaCache"
2. In the description, you mentioned other PRs, but you didn't clarify the relationship between this PR and the other PRs. I know that this one is a replacement for #341, but that's only because I asked you separately.

• Is the description for this correct? You said

  when a image does not possess a thumbnail uri, this pr would put Original & 400x400 into image viewer while https://github.com/project-robius/robrix/pull/341 would put Original & File into image viewer.

  But I do not think that's correct, since "original" and "file" should be the same thing, right?

[aaravlu]

But I do not think that's correct, since "original" and "file" should be the same thing, right?

original is uri, file is format, together they form the request parameters

/// Parameters for a request for retrieve media data.
///
/// This is used as a key in the media cache too.
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct MediaRequestParameters {
    /// The source of the media file.
    pub source: MediaSource,

    /// The requested format of the media data.
    pub format: MediaFormat,
}

/// The requested format of a media file.
#[derive(Clone, Debug, Serialize, Deserialize)]
pub enum MediaFormat {
    /// The file that was uploaded.
    File,

    /// A thumbnail of the file that was uploaded.
    Thumbnail(MediaThumbnailSettings),
}

original may macthing multi images

file still may macthing multi images

But one MediaRequestParameters must matching one image

[aaravlu]

A full title, such as "Add full-screen ImageViewer widget. Support storing original files and thumbnails in the MediaCache"

Yeah, you are right.

[aaravlu]

#341 is fully correct but too complex so i give up one rare condition to make #443 while it's more simple.

[kevinaboos]

original may macthing multi images

file still may macthing multi images

But one MediaRequestParameters must matching one image

I don't understand what this means.

[kevinaboos]

I've marked this as waiting-on-author since I have a lot of questions that need to be answered before I do another review round.

[aaravlu]

I don't understand what this means.

original, thumbnail is uri (MediaSource)

original uri always exists but thumbnail uri is Option.

100x100, 200x200, 300x300, File, etc is format (MediaFormat).

pub struct MediaRequestParameters {
    /// The source of the media file.
    pub source: MediaSource,

    /// The requested format of the media data.
    pub format: MediaFormat,
}

Neither the uri nor the format can determine exactly what an image is.

But they combine could.

The full-size you said is original (source) + File (format).

What we want is: Fetching thumbnail (source) + 400x400 (format) in timeline (replace it with original (source) + 400x400 (format) if no thumbnail source ). Fetching original (source) + File (format) in image viewer.

[aaravlu]

one for the full-size "original" image (always available), and possibly one for the thumbnail of that same image (sometimes available)

true

[aaravlu]

please restore my feature that returns the MediaFormat of the image that was requested, such that the caller can determine whether the thumbnail or full-size image was returned.

It is almost impossible unless we restore the input value (MediaFormat).

Note here, we just dont know which format we queried:

https://github.com/project-robius/robrix/blob/4dc2fae3e5ce206032cf89a0ac2cbd29fbf1cd84/src/media_cache.rs#L92-L94

But the fact is the function caller doesnot need to know which format he wants.

[aaravlu]

@kevinaboos See https://github.com/aaravlu/robrix/pull/2 and it's diff to know why we cant set_image_keys when occupied:

it's a test based on branch fix327-2

[kevinaboos]

@kevinaboos See aaravlu#2 and it's diff to know why we cant set_image_keys when occupied:

it's a test based on branch fix327-2

huh? I don't understand, and I don't think that's correct either.

It looks like it simply cannot be fetched because you're unconditionally overwriting the entire entry in the cache. So if it was already fetched, you've now deleted it....

Setting the relationship between image URI keys (i.e., establishing the relationship that a thumbnail URI and a full-size image URI are referring to the same image) is completely independent of setting the actual cached content of those images.

[kevinaboos]

At this point, I think it's best if we move on from this debate, as it has been ongoing for far too long. I will gladly accept your contributions for the ImageViewer widget and the design of the actions there (since those are now all good and correct), but the cache design simply isn't right, and that's an objective fact, not just my opinion.

I would recommend that you select another topic to work on. I will make the necessary changes to the remainder of this PR and merge it in on my own time later.

[aaravlu]

I will make the necessary changes to the remainder of this PR and merge it in on my own time later.

Thanks, it's supposed to be my job, but now it really bothering you.

because you're unconditionally overwriting the entire entry in the cache

Yeah, Sorry for my incorrect explanation.

It just keeps being overwrited rather than not fetched.

I would recommend that you select another topic to work on.

I am working on https://github.com/project-robius/robrix/pull/396 recently

I think it's best if we move on from this debate

Fully agree.

[aaravlu]

@kevinaboos

Please rebase this pr's commit messages to one before you merge this pr. Thanks.

[kevinaboos]

this is blocked on me, so i'll add the waiting-on-review label to remind myself. Will be a while before I get to it, though.

[kevinaboos]

This is still waiting on me, so let's leave the waiting-on-review label in place. However, it'll be a while before I can get to it, probably early June.