Use devcontainer with php v8.2 and readd preview images to tables

[MI-KY]

Modified the devcontainer.json to use a container with php v8.2 .
Readded thumbnailUrl and icon so that this data is not dropped anymore when a url to for example an image is saved. I hope this was not intentionally removed by #779 but the result seems to be that actually no preview is shown...

Sorry, I wanted to create two PRs but I had some problems because of DCO and I'm not used to this 🙂

Resolves #1392
Resolves #1375

[enjeck]

Thanks!

[enjeck]

I notice that it works for newly created image links, but pre-existing links do not show the previews. But we could handle this in a separate PR

[MI-KY]

You're right the change only affects new links because it seems that since this two lines have been missing every link written to the database lacks of a thumbnailUrl (at least in the last versions of tables)... So solving that would maybe need some lookup for links without a thumbnailUrl and then generating / searching the correct one (?).

[MI-KY]

Hi,
please excuse my question, but do I have to do something else to push forward the PR?

[enjeck]

Hi, please excuse my question, but do I have to do something else to push forward the PR?

There's a failing test: https://github.com/nextcloud/tables/actions/runs/11084225046/job/30817689205?pr=1393. Since we changed added more array keys in the code, we'll need to add them to the tests as well at tests/unit/Service/ColumnTypes/TextLinkBusinessTest.php too. I'm not yet sure what the right thumbnailUrl and icon values that would make the tests pass

[MI-KY]

Thanks a lot for your answer, I will try to solve this if I can :-) Am 7. Oktober 2024 20:33:22 MESZ schrieb Cleopatra ***@***.***>:

…

> Hi, please excuse my question, but do I have to do something else to push forward the PR? There's a failing test: https://github.com/nextcloud/tables/actions/runs/11084225046/job/30817689205?pr=1393. Since we changed added more array keys in the code, we'll need to add them to the tests as well at `tests/unit/Service/ColumnTypes/TextLinkBusinessTest.php` too. I'm not yet sure what the right `thumbnailUrl` and `icon` values that would make the tests pass -- Reply to this email directly or view it on GitHub: #1393 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[MI-KY]

Hi @enjeck ,

I tried my best to make the phpunit tests work again but I have to admit that I didn't had much knowledge about this tests before. I've read the documentation and I think that my corrections should be correct as after my changes parseValue inside TextLinkBusinessTest.php should always return the values for thumbnailUrl and icon.

But as said: I hope this is correct and I didn't have done something stupid 🙂

[MI-KY]

I would like to add something that is connected with this PR although this was not introduced by my commits but was always a way Tables saved the links to the preview images in thumbnailUrl inside the database: Tables saved this links as whole url like https://cloud.example.com/index.php/..... (see

tables/src/shared/components/ncTable/partials/rowTypePartials/TextLinkForm.vue

Line 208 in ee573ae

res = await axios.get(generateOcsUrl('/search/providers/' + providerId + '/search?term=' + term))

)
For internal links to files inside Nextcloud this works but seams to get broken if for example the subdomain or port of the Nextcloud changes - afterwards all links inside thumbnailUrl that worked before get invalid and point to a non existing url...

[enjeck]

For internal links to files inside Nextcloud this works but seams to get broken if for example the subdomain or port of the Nextcloud changes - afterwards all links inside thumbnailUrl that worked before get invalid and point to a non existing url...

This is a great observation, thanks for pointing it out. I notice that even the value property uses the full url, so it does break if the subdomain changes. Here's an example of the data we save:

{"thumbnailUrl":"http:\/\/nextcloud.local\/index.php\/core\/preview?fileId=10&x=32&y=32",
"icon":"\/index.php\/apps\/theming\/img\/core\/filetypes\/image.svg?v=87d39db1",
"title":"photo-1503991721143-75f95ebf1e55.jpeg",
"value":"http:\/\/nextcloud.local\/index.php\/f\/10",
"providerId":"url"}

We could have gotten away with only saving the file id (the number after \/f in http:\/\/nextcloud.local\/index.php\/f\/10), but it's too late now, I think. We already have users with the full url, so we can't easily switch to just using the file Id here, which would be more stable as we don't expect it to change. Maybe there's a way to go about it I'm not aware of @blizzz @juliusknorr ??

I just realized that we don't need to store the additional thumbnailUrl and icon properties. By using value which has the fileId, we can get the thumbnailUrl. The icon url is fixed based on the users theme. I made a branch with my experiment: main...img-previews . I prefer this approach because it will also fix the problem with pre-existing links I highlighted here: #1393 (comment)

[enjeck]

@MI-KY Do you want to continue working on this or should I take over? I realize it'll require more discussions and changes that you might not have bandwidth for. Either way, I think we should split this PR up, since it's solving multiple issues. Then we can get the devcontainer fix (2e3089b) merged in and continue with the image previews separately.

[MI-KY]

Honestly I think that value needs the full url in some cases - when we link to an external url (at least I think that this is possible with some search providers?). But you're completely right for internal urls to files where the same problem could arise like for the thumbnails.

The idea with getting the thumbnail using the value is great because we wouldn't have the other problems when changing the subdomain ecc. 🙂 . But in this case I think that we need some additional information if the link is pointing to an internal file (and therefore a thumbnail can be loaded) or it is pointing to an external link...?
When the thumbnail url is retrieved by value maybe also the x and y value in that url should (and would) be "dynamic" so that in a future there would exist the possibility to add a bigger preview like I described in #1404 👍 .

I'm happy if you take over because I'm new to Nextcloud development and therefore maybe this is getting a bit to big for me at the moment 👍 🙂 .

[enjeck]

Honestly I think that value needs the full url in some cases - when we link to an external url (at least I think that this is possible with some search providers?). But you're completely right for internal urls to files where the same problem could arise like for the thumbnails

Another good point that I didn't consider. I'd have to check, thanks!

[enjeck]

@MI-KY Hmm, I tried to split this up, but it seems like I can't make changes to your branch. Can you please remove every commit except the first (2e3089b) which fixes the devcontainer so that I can merge that?

Then I will copy the other changes to a new PR and continue from there.

If removing commits it too complicated, then I can just close this and copy everything to separate PRs instead

[MI-KY]

Hi @enjeck ,
as you see I tried but I think now I have messed it up completely 🤦‍♂️.
It would be great if you could split the PR up into new ones - for me it's no problem to "loose" the credits of creating the pull request 🙂 . Sorry for the mess, I will dive deeper into the git documentation to understand what happened...