Skip to content
This repository has been archived by the owner on Oct 17, 2019. It is now read-only.

Implement #132: Support pasting images into room #138

Closed
wants to merge 5 commits into from
Closed

Implement #132: Support pasting images into room #138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a2cc88
Implement #132: Support pasting images into room
christarazi Nov 25, 2017
0e178cc
Use QKeySequence::Copy instead of hardcoded ctrl-c
christarazi Nov 26, 2017
af238f7
Add ability to embed images and text in same message
christarazi Nov 26, 2017
41b887b
Remove unneeded explicit call to insertFromMimeData()
christarazi Nov 27, 2017
4e8034c
Fix case for X11 users when copying an image
christarazi Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/TextInputWidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,21 @@ class FilteredTextEdit : public QTextEdit
void stoppedTyping();
void message(QString);
void command(QString name, QString args);
void image(QString name);

protected:
void keyPressEvent(QKeyEvent *event) override;
bool canInsertFromMimeData(const QMimeData *source) const override;
void insertFromMimeData(const QMimeData *source) override;

private:
std::deque<QString> true_history_, working_history_;
size_t history_index_;
QTimer *typingTimer_;

QString pastedImagePath_;
bool isImage_;

void textChanged();
void afterCompletion(int);
};
Expand Down
48 changes: 48 additions & 0 deletions src/TextInputWidget.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@
*/

#include <QAbstractTextDocumentLayout>
#include <QApplication>
#include <QClipboard>
#include <QDebug>
#include <QFile>
#include <QFileDialog>
#include <QImageReader>
#include <QMimeData>
#include <QPainter>
#include <QStyleOption>

Expand All @@ -31,6 +34,7 @@ static constexpr size_t INPUT_HISTORY_SIZE = 127;
FilteredTextEdit::FilteredTextEdit(QWidget *parent)
: QTextEdit{parent}
, history_index_{0}
, isImage_{false}
{
connect(document()->documentLayout(),
&QAbstractTextDocumentLayout::documentSizeChanged,
Expand Down Expand Up @@ -93,12 +97,50 @@ FilteredTextEdit::keyPressEvent(QKeyEvent *event)
}
break;
}
case Qt::Key_V: {
if (event->modifiers() == Qt::ControlModifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Ralith Ralith Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@christarazi christarazi Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@christarazi christarazi Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@christarazi christarazi Nov 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

insertFromMimeData(QApplication::clipboard()->mimeData());
}
break;
}
default:
QTextEdit::keyPressEvent(event);
break;
}
}

bool
FilteredTextEdit::canInsertFromMimeData(const QMimeData *source) const
{
return (source->hasImage() || source->hasText() ||
QTextEdit::canInsertFromMimeData(source));
}

void
FilteredTextEdit::insertFromMimeData(const QMimeData *source)
{
if (source->hasImage()) {
QImage img = qvariant_cast<QImage>(source->imageData());
pastedImagePath_ = QDir::tempPath() + '/' + "nheko_pasted_img.png";

// Save image into temporary path to be loaded later.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@Ralith Ralith Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

QFile file{pastedImagePath_};
if (!file.open(QIODevice::WriteOnly)) {
qDebug() << "Failed to create temporary image file";
return;
}
if (!img.save(file.fileName(), "PNG")) {
qDebug() << "Failed to save image data";
return;
}

isImage_ = true;
textCursor().insertImage(pastedImagePath_);
} else {
QTextEdit::insertFromMimeData(source);
}
}

void
FilteredTextEdit::stopTyping()
{
Expand Down Expand Up @@ -155,6 +197,9 @@ FilteredTextEdit::submit()
} else {
command(name, args);
}
} else if (isImage_) {
emit image(pastedImagePath_);
isImage_ = false;
} else {
message(std::move(text));
}
Expand Down Expand Up @@ -229,6 +274,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);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

connect(emojiBtn_,
SIGNAL(emojiSelected(const QString &)),
this,
Expand Down