Implement #132: Support pasting images into room #138

christarazi · 2017-11-25T21:47:48Z

Implements #132 to allow pasting of images using ctrl-v.

Ralith

In addition to the other issues I highlighted, I don't see any graceful handling for the case where a user attempts to enter both text and imagery in the same message. It looks like text entered alongside an image is accepted and then silently discarded, which is a bad user experience.

Ralith · 2017-11-26T01:27:13Z

src/TextInputWidget.cc

@@ -93,12 +97,50 @@ FilteredTextEdit::keyPressEvent(QKeyEvent *event)
                }
                break;
        }
+        case Qt::Key_V: {
+                if (event->modifiers() == Qt::ControlModifier) {


Hardcoding this is incorrect. You cannot make assumptions about what keybind any given system uses to express "paste." Try using QKeySequence::Copy instead.

On further review, given that Qt handles MIME clipboards in the first place, it's actually not obvious to me that handling pasting by hand like this is even necessary at all, given the other changes in this PR. If it truly is, some comments explaining why Qt's default behavior is inadequate would be nice.

I'm unsure of what you mean. If you mean, that Qt has built in support out-of-the-box for pasting images into a QTextEdit, then I have not seen any document which suggests that. The fact that QTextEdit::insertFromMimeData and QTextEdit::canInsertFromMimeData exist suggest that it does not have built in support. However, this is just an educated guess.

No need to guess. What happens if you disable this hardcoding but retain the overrides to those methods?

Good call. I removed the explicit call to insertFromMimeData() (the entire case in the switch statement), and kept just the overrides. It still works.

Unnecessary hardcoded handling of paste fixed in 41b887b and 0e178cc

Ralith · 2017-11-26T01:28:00Z

src/TextInputWidget.cc

+                QImage img = qvariant_cast<QImage>(source->imageData());
+                pastedImagePath_ = QDir::tempPath() + '/' + "nheko_pasted_img.png";
+
+                // Save image into temporary path to be loaded later.


There's no reason for this to hit disk, and doing so introduces a number of bugs. Leave the image in memory.

The reason I write to disk is because MatrixClient::uploadImage() takes in a QString that represents a file path to an image. This means I would have to overload uploadImage or create a new function which accepts an image from memory. If you think that is worth doing, I will implement it.

You should probably refactor uploadImage to accept data from arbitrary sources, e.g. by taking a QIODevice. This change to the PR is necessary for a number of reasons; most immediately, the current code introduces system-global state, which makes it unsafe to have multiple instances of the class even in entirely different processes. It is also poor behavior to silently re-encode submitted images.

I'll create a new pull request with the requested changes.

Ralith · 2017-11-26T23:27:50Z

Another concern is the handling of pastes that include both image(s) and text concatenated in a single piece of MIME data, along with a possible alternative plaintext representation.

It's worth note that Matrix can support this type of mixed-content message by placing the alternative plaintext in body and using formatted_body with img tags that reference mxc URIs to encode the mixed form, although no existing client generates such messages automatically. I believe Riot, at least, will nonetheless render such messages as expected.

christarazi · 2017-11-27T01:38:51Z

Another concern is the handling of pastes that include both image(s) and text concatenated in a single piece of MIME data, along with a possible alternative plaintext representation.

It's worth note that Matrix can support this type of mixed-content message by placing the alternative plaintext in body and using formatted_body with img tags that reference mxc URIs to encode the mixed form, although no existing client generates such messages automatically. I believe Riot, at least, will nonetheless render such messages as expected.

@Ralith On my local branch, I made the change to allow both images and text in the same message. I did this by checking if isImage_ is true, and if so, then removing the encoded image (encoded as U+FFFC) from the text, and allowing the existing code to take care of the rest.

Edit:

Fixed in af238f7.

mujx · 2017-11-27T14:15:19Z

How exactly I can use this feature? If I copy-paste an image into the text field I just get the file path of that image and nothing happens when I hit enter.

christarazi · 2017-11-28T06:40:48Z

How exactly I can use this feature? If I copy-paste an image into the text field I just get the file path of that image and nothing happens when I hit enter.

Didn't realize that X11 treats copying files differently in regards to setting the MIME type. I was testing my implementation by right-clicking an image in my browser and hitting "Copy image". This is now fixed in 4e8034c.

christarazi · 2017-11-28T06:52:34Z

Now that I think of it, we might as well handle all files that are pasted in the text box. The commit in 4e8034c only handles pasting images, but it should be a trivial change to allow pasting of any file.

mujx · 2017-11-28T12:26:24Z

This also doesn't work on macOS. Instead of the actual image a generic image placeholder is sent.

Another issue is that the image is rendered in the text field and only a thin slice of it is visible which makes it a bad user experience. It would be better to show a confirmation popup to send the image or file (like Telegram does).

I'm against mixing images and text in the same messages cause it would be quite difficult to implement and it doesn't have any real benefit in my opinion.

christarazi · 2017-11-28T15:50:47Z

Interesting, seems like handling MIME data was more convoluted than I thought. I will look into it. You can scroll using the mouse wheel to see the image, but I agree, this can be improved. Alright, I can revert that commit if that's what you'd prefer.

…

On November 28, 2017 4:26:25 AM PST, mujx ***@***.***> wrote: This also doesn't work on macOS. Instead of the actual image a generic image placeholder is sent. Another issue is that the image is rendered in the text field and only a thin slice of it is visible which makes it a bad user experience. It would be better to show a confirmation popup to send the image or file (like Telegram does). I'm against mixing images and text in the same messages cause it would be quite difficult to implement and it doesn't have any real benefit in my opinion. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #138 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

christarazi · 2017-11-29T15:30:05Z

Do you know of a method to test macOS binaries on Linux? I do not own a Mac.

…

On November 28, 2017 4:26:25 AM PST, mujx ***@***.***> wrote: This also doesn't work on macOS. Instead of the actual image a generic image placeholder is sent. Another issue is that the image is rendered in the text field and only a thin slice of it is visible which makes it a bad user experience. It would be better to show a confirmation popup to send the image or file (like Telegram does). I'm against mixing images and text in the same messages cause it would be quite difficult to implement and it doesn't have any real benefit in my opinion. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #138 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

mujx · 2017-11-29T18:17:41Z

@christarazi I could find a fix for mac later, it's not super critical. The important part is to have that popup when a pasted file or image is detected and possibly allow the user to edit the name of the file before sending it, so nothing would be pasted into the text field.

Ralith

I agree with @mujx that (opening a dialog for) sending pasted (or dropped) files immediately makes more sense than handling them as if they were part of the input text, at least so long as nheko doesn't have advanced formatting capabilities.

Ralith · 2017-12-12T23:05:14Z

src/TextInputWidget.cc

@@ -229,6 +280,9 @@ TextInputWidget::TextInputWidget(QWidget *parent)
        connect(sendFileBtn_, SIGNAL(clicked()), this, SLOT(openFileSelection()));
        connect(input_, &FilteredTextEdit::message, this, &TextInputWidget::sendTextMessage);
        connect(input_, &FilteredTextEdit::command, this, &TextInputWidget::command);
+        connect(input_, &FilteredTextEdit::image, this, [=](QString name) {
+                emit uploadImage(name);
+        });


I believe this can be written connect(input_, &FilteredTextEdit::image, this, uploadImage)

christarazi · 2017-12-24T19:11:59Z

I've created a new PR (#180) which adds a preview dialog for pasting images and also modifies the MatrixClient::upload*() members to take in a QIODevice instead of a QString.

Implement mujx#132: Support pasting images into room

1a2cc88

Ralith suggested changes Nov 26, 2017

View reviewed changes

christarazi added 2 commits November 25, 2017 23:08

Use QKeySequence::Copy instead of hardcoded ctrl-c

0e178cc

Add ability to embed images and text in same message

af238f7

Remove unneeded explicit call to insertFromMimeData()

41b887b

Fix case for X11 users when copying an image

4e8034c

Ralith reviewed Dec 12, 2017

View reviewed changes

christarazi mentioned this pull request Dec 24, 2017

[Updated] Implements #132: Pasting images into room #180

Merged

christarazi closed this Dec 24, 2017

christarazi deleted the paste-img branch February 19, 2018 05:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement #132: Support pasting images into room #138

Implement #132: Support pasting images into room #138

christarazi commented Nov 25, 2017

Ralith left a comment •

edited

Loading

Ralith Nov 26, 2017

Ralith Nov 26, 2017 •

edited

Loading

christarazi Nov 26, 2017 •

edited

Loading

Ralith Nov 26, 2017

christarazi Nov 27, 2017 •

edited

Loading

christarazi Nov 28, 2017 •

edited

Loading

Ralith Nov 26, 2017

christarazi Nov 26, 2017

Ralith Nov 26, 2017 •

edited

Loading

christarazi Nov 26, 2017

Ralith commented Nov 26, 2017

christarazi commented Nov 27, 2017 •

edited

Loading

mujx commented Nov 27, 2017

christarazi commented Nov 28, 2017 •

edited

Loading

christarazi commented Nov 28, 2017

mujx commented Nov 28, 2017

christarazi commented Nov 28, 2017 via email

christarazi commented Nov 29, 2017 via email

mujx commented Nov 29, 2017

Ralith left a comment

Ralith Dec 12, 2017

christarazi commented Dec 24, 2017

Implement #132: Support pasting images into room #138

Implement #132: Support pasting images into room #138

Conversation

christarazi commented Nov 25, 2017

Ralith left a comment • edited Loading

Choose a reason for hiding this comment

Ralith Nov 26, 2017

Choose a reason for hiding this comment

Ralith Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

christarazi Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

Ralith Nov 26, 2017

Choose a reason for hiding this comment

christarazi Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

christarazi Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Ralith Nov 26, 2017

Choose a reason for hiding this comment

christarazi Nov 26, 2017

Choose a reason for hiding this comment

Ralith Nov 26, 2017 • edited Loading

Choose a reason for hiding this comment

christarazi Nov 26, 2017

Choose a reason for hiding this comment

Ralith commented Nov 26, 2017

christarazi commented Nov 27, 2017 • edited Loading

mujx commented Nov 27, 2017

christarazi commented Nov 28, 2017 • edited Loading

christarazi commented Nov 28, 2017

mujx commented Nov 28, 2017

christarazi commented Nov 28, 2017 via email

christarazi commented Nov 29, 2017 via email

mujx commented Nov 29, 2017

Ralith left a comment

Choose a reason for hiding this comment

Ralith Dec 12, 2017

Choose a reason for hiding this comment

christarazi commented Dec 24, 2017

Ralith left a comment •

edited

Loading

Ralith Nov 26, 2017 •

edited

Loading

christarazi Nov 26, 2017 •

edited

Loading

christarazi Nov 27, 2017 •

edited

Loading

christarazi Nov 28, 2017 •

edited

Loading

Ralith Nov 26, 2017 •

edited

Loading

christarazi commented Nov 27, 2017 •

edited

Loading

christarazi commented Nov 28, 2017 •

edited

Loading