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

fix: extend/fix DND and clipboard image operations #10975

Merged
merged 6 commits into from
Jun 8, 2023

Conversation

caybro
Copy link
Member

@caybro caybro commented Jun 7, 2023

What does the PR do

Fixes several issues/deficiencies we had with handling image data (paste, drag'n'drop, resizing); pls see the individual commits for explanation; namely:

  • fix crash when dropping a remote URL (non-Chrome browsers)
  • fix DND checks for URLs containing a query
  • missing support for dropping URLs
  • fixup support for dropping URLs from Chrome (uses a non standard MIME type)
  • fix missing support for dropping a bunch of files (e.g. from a file manager)
  • more efficient image_resizer C++ code, more robust file size/extension checks

Affected areas

StatusChatInput, sendImages

Screenshot of functionality (including design for comparison)

  • I've checked the design and this PR matches it

TODO: video

@caybro caybro linked an issue Jun 7, 2023 that may be closed by this pull request
@caybro caybro requested a review from jrainville June 7, 2023 16:14
@status-im-auto
Copy link
Member

status-im-auto commented Jun 7, 2023

Jenkins Builds

Click to see older builds (28)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 0e1a4f2 #1 2023-06-07 16:18:39 ~5 min tests/nim 📄log
✔️ 0e1a4f2 #1 2023-06-07 16:18:51 ~5 min tests/imports 📄log
✔️ 0e1a4f2 #1 2023-06-07 16:22:55 ~10 min macos/aarch64 🍎dmg
✖️ 0e1a4f2 #1 2023-06-07 16:23:17 ~10 min tests/e2e 📄log
✔️ 0e1a4f2 #1 2023-06-07 16:24:01 ~11 min macos/x86_64 🍎dmg
✔️ 0e1a4f2 #1 2023-06-07 16:47:47 ~34 min windows/x86_64 💿exe
✔️ 09c02d0 #2 2023-06-07 22:06:59 ~4 min tests/imports 📄log
✔️ 09c02d0 #2 2023-06-07 22:07:47 ~5 min tests/nim 📄log
✔️ 09c02d0 #2 2023-06-07 22:07:52 ~5 min macos/aarch64 🍎dmg
✖️ 09c02d0 #2 2023-06-07 22:10:02 ~7 min tests/e2e 📄log
✔️ 09c02d0 #2 2023-06-07 22:13:25 ~11 min macos/x86_64 🍎dmg
✔️ 09c02d0 #2 2023-06-07 22:29:22 ~27 min windows/x86_64 💿exe
✖️ 09c02d0 #3 2023-06-07 22:32:06 ~6 min tests/e2e 📄log
✖️ 09c02d0 #4 2023-06-07 23:13:15 ~5 min tests/e2e 📄log
✖️ 09c02d0 #5 2023-06-08 00:20:27 ~6 min tests/e2e 📄log
✔️ 3f7460f #3 2023-06-08 00:34:08 ~5 min tests/imports 📄log
✔️ 3f7460f #3 2023-06-08 00:34:25 ~5 min tests/nim 📄log
✔️ 3f7460f #3 2023-06-08 00:34:31 ~5 min macos/aarch64 🍎dmg
✖️ 3f7460f #6 2023-06-08 00:35:32 ~6 min tests/e2e 📄log
✔️ 3f7460f #3 2023-06-08 00:38:03 ~8 min macos/x86_64 🍎dmg
✔️ 3f7460f #3 2023-06-08 00:59:51 ~30 min windows/x86_64 💿exe
✔️ ca26330 #4 2023-06-08 07:14:31 ~4 min tests/imports 📄log
✔️ ca26330 #4 2023-06-08 07:15:30 ~5 min tests/nim 📄log
✖️ ca26330 #7 2023-06-08 07:16:21 ~6 min tests/e2e 📄log
✔️ ca26330 #4 2023-06-08 07:18:35 ~8 min macos/x86_64 🍎dmg
✔️ ca26330 #4 2023-06-08 07:25:10 ~15 min macos/aarch64 🍎dmg
✔️ ca26330 #4 2023-06-08 07:35:07 ~25 min windows/x86_64 💿exe
✔️ ca26330 #8 2023-06-08 08:47:40 ~38 min tests/e2e 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f9b7ea4 #5 2023-06-08 09:02:59 ~5 min tests/imports 📄log
✔️ f9b7ea4 #5 2023-06-08 09:04:21 ~6 min tests/nim 📄log
✖️ f9b7ea4 #9 2023-06-08 09:07:26 ~9 min tests/e2e 📄log
✔️ f9b7ea4 #5 2023-06-08 09:08:16 ~10 min macos/x86_64 🍎dmg
✔️ f9b7ea4 #5 2023-06-08 09:12:05 ~14 min macos/aarch64 🍎dmg
✔️ c5d5d1e #6 2023-06-08 09:28:09 ~7 min tests/imports 📄log
✔️ c5d5d1e #6 2023-06-08 09:28:45 ~8 min tests/nim 📄log
✔️ c5d5d1e #6 2023-06-08 09:31:24 ~10 min macos/x86_64 🍎dmg
✔️ c5d5d1e #6 2023-06-08 09:33:18 ~12 min macos/aarch64 🍎dmg
✔️ c5d5d1e #6 2023-06-08 09:52:16 ~31 min windows/x86_64 💿exe
✔️ c5d5d1e #10 2023-06-08 09:55:15 ~34 min tests/e2e 📄log

@jrainville
Copy link
Member

I tried this PR and drag and drop works fine (though it did work for me anyway before).

Pasting from Brave still doesn't work though. So #10926 is not fixed by this.

I feel like this is a Brave/Qt Bug that we can't really fix

@caybro
Copy link
Member Author

caybro commented Jun 7, 2023

I tried this PR and drag and drop works fine (though it did work for me anyway before).

Pasting from Brave still doesn't work though. So #10926 is not fixed by this.

I feel like this is a Brave/Qt Bug that we can't really fix

Ah, too bad. Most likely it is a Qt bug, fancy updating your Qt version? :) I'll install Brave myself anyway and will give it a try

@caybro caybro force-pushed the 10926-fix-dnd-clipboard-imageresizer branch from 0e1a4f2 to 09c02d0 Compare June 7, 2023 22:02
@caybro
Copy link
Member Author

caybro commented Jun 7, 2023

I tried this PR and drag and drop works fine (though it did work for me anyway before).

Pasting from Brave still doesn't work though. So #10926 is not fixed by this.

I feel like this is a Brave/Qt Bug that we can't really fix

@jrainville works for me :/

Copy & paste:
image

@caybro
Copy link
Member Author

caybro commented Jun 7, 2023

@jakubgs the e2e build fails here repeatedly with at least some meaningful error message:

[2023-06-07T22:32:04.913Z] /usr/bin/ld: nimcache/release/nim_status_client/@m..@svendor@snimbus-build-system@svendor@sNim@slib@spure@snet.nim.c.o: in function `checkCertName__pureZnet_1309':
[2023-06-07T22:32:04.913Z] /home/jenkins/workspace/_linux_x86_64_tests-e2e_PR-10975/vendor/nimbus-build-system/vendor/Nim/lib/pure/net.nim:831: undefined reference to `SSL_get_peer_certificate'
[2023-06-07T22:32:04.913Z] collect2: error: ld returned 1 exit status
[2023-06-07T22:32:04.913Z] Error: execution of an external program failed: 'g++  @nim_status_client_linkerArgs.txt'
[2023-06-07T22:32:04.913Z] make: *** [Makefile:476: bin/nim_status_client] Error 1
script returned exit code 2

Some stale nimcache? OpenSSL upgraded?

@caybro caybro marked this pull request as ready for review June 8, 2023 00:12
- getFileSize: NIM version would crash on non-existing or remote files
- isValidImageUrl: properly detect file extensions when the URL contains
a query (eg "file.jpeg?width=1000&height=600")
- when verifying whether an image has a suitable size or extension
- accept both the blob (`data:image/jpeg;base64` payload) or a path/URL
to a local file
- remove the usage of QPixmap, QImage is enough and more suitable for
save/load and resizing as well
- remove usage of an extra file when saving
- when the user drops a remote URL (e.g. from a webbrowser), detect it
and download it before passing onto the "image_resizer"
- since the "dos_image_resizer" now handles both data and URLs, we can
simply forward the `imageUrl` to it
- detect an invalid (empty) result from "dos_image_resizer" and don't
try to send such an image
@caybro caybro force-pushed the 10926-fix-dnd-clipboard-imageresizer branch from 09c02d0 to 3f7460f Compare June 8, 2023 00:28
@alexjba
Copy link
Contributor

alexjba commented Jun 8, 2023

I tried this PR and drag and drop works fine (though it did work for me anyway before).
Pasting from Brave still doesn't work though. So #10926 is not fixed by this.
I feel like this is a Brave/Qt Bug that we can't really fix

@jrainville works for me :/

Copy & paste: image

Works on MacOs with Safari and Chrome

@caybro
Copy link
Member Author

caybro commented Jun 8, 2023

@jrainville works for me :/

Works on MacOs with Safari and Chrome

@alexjba Qt version?

- most browsers will pass a standard `text/uri-list` MIME type, however
Chrome uses `text/x-moz-url` so we handle that separately
(`drop.hasUrls` is `false` in this case)
@caybro caybro force-pushed the 10926-fix-dnd-clipboard-imageresizer branch from 3f7460f to ca26330 Compare June 8, 2023 07:09
@alexjba
Copy link
Contributor

alexjba commented Jun 8, 2023

@jakubgs the e2e build fails here repeatedly with at least some meaningful error message:

[2023-06-07T22:32:04.913Z] /usr/bin/ld: nimcache/release/nim_status_client/@m..@svendor@snimbus-build-system@svendor@sNim@slib@spure@snet.nim.c.o: in function `checkCertName__pureZnet_1309':
[2023-06-07T22:32:04.913Z] /home/jenkins/workspace/_linux_x86_64_tests-e2e_PR-10975/vendor/nimbus-build-system/vendor/Nim/lib/pure/net.nim:831: undefined reference to `SSL_get_peer_certificate'
[2023-06-07T22:32:04.913Z] collect2: error: ld returned 1 exit status
[2023-06-07T22:32:04.913Z] Error: execution of an external program failed: 'g++  @nim_status_client_linkerArgs.txt'
[2023-06-07T22:32:04.913Z] make: *** [Makefile:476: bin/nim_status_client] Error 1
script returned exit code 2

Some stale nimcache? OpenSSL upgraded?

There's been a discussion on this here: #9428 (comment)

It's due importing httpclient in nim: https://github.com/status-im/status-desktop/pull/10975/files#diff-09c38470e3ca52e97f3e41aec8963c42be8b404a260f913ff24fb2930dd3b76eR2

Now we have a newer nim version AFAIK so there's a chance we can fix it. But we need to be careful not to link two different OpenSSL versions in the app because NIM doesn't support this. Using this flag could fix it: https://github.com/nim-lang/Nim/blob/devel/lib/wrappers/openssl.nim#L40. We could try to force the same version we're already linking static or dynamic, depending on platform.

Another option is to do the httpClient logic in qml, c++ or go.

@alexjba
Copy link
Contributor

alexjba commented Jun 8, 2023

@jrainville works for me :/

Works on MacOs with Safari and Chrome

@alexjba Qt version?

The same version used on CI, 5.15.8.

Copy link
Contributor

@alexjba alexjba left a comment

Choose a reason for hiding this comment

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

LGTM!

by explicitely specifying `-d:useOpenssl3`

cf #9428 (comment)
@caybro caybro force-pushed the 10926-fix-dnd-clipboard-imageresizer branch from f9b7ea4 to c5d5d1e Compare June 8, 2023 09:20
@caybro caybro merged commit 1c0a712 into master Jun 8, 2023
@caybro caybro deleted the 10926-fix-dnd-clipboard-imageresizer branch June 8, 2023 13:33
@jrainville
Copy link
Member

The same version used on CI, 5.15.8.

If I use 0.12, I can still reproduce the issue. Is there any chance that we updated the QT version since then on CI?

@caybro
Copy link
Member Author

caybro commented Jun 8, 2023

The same version used on CI, 5.15.8.

If I use 0.12, I can still reproduce the issue. Is there any chance that we updated the QT version since then on CI?

Yeah I think we did, not sure if for all OSs

@alexjba
Copy link
Contributor

alexjba commented Jun 8, 2023

The same version used on CI, 5.15.8.

If I use 0.12, I can still reproduce the issue. Is there any chance that we updated the QT version since then on CI?

AFAIK only MacOs is using 5.15.8 since 0.11. Linux is still stuck to 5.15.2 (#9350)
Not sure about Windows tho..

@jrainville
Copy link
Member

Interesting. I'm using 5.15.2 locally too, so that might explain why I also see the issue reproducing. I'll try updating to 5.15.8

@jrainville
Copy link
Member

jrainville commented Jun 8, 2023

Dumb question, how did you install 5.15.8 @caybro ? The online installer only gives access to 5.15.2 and 6.x

@caybro
Copy link
Member Author

caybro commented Jun 8, 2023

Dumb question, how did you install 5.15.8 @lukaszso ? The online installer only gives access to 5.15.2 and 6.x

Comes with my Linux distro :)

@caybro
Copy link
Member Author

caybro commented Jun 8, 2023

Dumb question, how did you install 5.15.8 @lukaszso ? The online installer only gives access to 5.15.2 and 6.x

Comes with my Linux distro :)

Otherwise there are just source packages to download

@alexjba
Copy link
Contributor

alexjba commented Jun 9, 2023

Dumb question, how did you install 5.15.8 @caybro ? The online installer only gives access to 5.15.2 and 6.x

On MacOs we're using Homebrew to install it. It's also available on linux, so you could try with brew install qt@5. I remember it was working fine on Docker. But I see the package is now upgraded to 5.15.9.

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.

[StatusChatInput] Some images do not get pasted in the chat input
5 participants