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

christarazi · 2017-12-24T19:10:58Z

This is an update to PR #138

mujx · 2017-12-24T20:07:46Z

Thanks a lot for your work! The preview modal is really good 👍

The only issue I spotted was with the file naming. On the dialog the default name should be the local filename. Also when I send the image it appears with the temporary filename and not with the one I gave. You can pass around the full local path, I don't think there is a need to create a temporary file.

Ralith

Thanks for your work! There are some issues remaining. In addition to the specific notes on the code, the message for the first commit is confusing; most of the changes it describes no longer apply.

Ralith · 2017-12-24T20:25:22Z

include/MatrixClient.h

-        void audioUploaded(const QString &roomid, const QString &filename, const QString &url);
+        void imageUploaded(const QString &roomid, QSharedPointer<QFile> file, const QString &url);
+        void fileUploaded(const QString &roomid, QSharedPointer<QFile> file, const QString &url);
+        void audioUploaded(const QString &roomid, QSharedPointer<QFile> file, const QString &url);


Why are these QFile? The whole point of the change to use QIODevice is that the uploaded data is not necessarily a file. As the filename (and only the filename) is needed for UI purposes, this should remain a QString.

Ah this is an oversight on my end. You are correct, they do not need to take a QFile. However, it is not clear to me what the QString should be, if the data does not come from a file (where the string would obviously be the file name).

I'd just invent something descriptive, depending on the actual origin of the data. For example, this PR might use the name "clipboard.png" for UI and upload filename purposes.

Just to clarify, how would I know if I received a file or a buffer containing data? Would the best method be to try to cast to a QFile and use the file name if it exists, and if the cast fails, then use the "clipboard.png" mentioned?

You should not need to know what specific QIODevice generic code receives; doing so defeats the purpose of having generic code. For example, the approach you outline will break down once there are non-clipboard sources of in-memory data (e.g. imagery captured directly from a camera). Justifiable uses of dynamic casting in C++ are extremely rare.

Instead, whenever a filename will eventually be needed alongside a QIODevice, you should pass a filename along from the very beginning. For example, the signature of FilteredTextEdit::image could be changed to void FilteredTextEdit::image(QSharedPointer<QIODevice> data, const QString &filename)

Ralith · 2017-12-24T20:28:27Z

include/MatrixClient.h

-        void uploadAudio(const QString &roomid, const QString &filename);
+        void uploadImage(const QString &roomid, QSharedPointer<QIODevice> iodev);
+        void uploadFile(const QString &roomid, QSharedPointer<QIODevice> iodev);
+        void uploadAudio(const QString &roomid, QSharedPointer<QIODevice> iodev);


Why do these need to be shared pointers?

They're shared pointers because notice the lambda in the MatrixClient::upload*() member functions for responding to a QNetworkReply::finished signal. By the time that lambda is executed (asynchronously), we have lost the scope of the QIODevice. If I were to pass by const ref, the reference would be bound to a temporary local, which would cause a segfault somewhere during the execution of the lambda. To resolve this, the best method I could think of was to create a pointer so that it's allocated on the heap.

I would love to hear if you have a better method.

Ralith · 2017-12-24T20:33:06Z

src/TextInputWidget.cc

+
+        emit image(file);
+
+        previewDialog_->close();


Since you close the preview dialog no matter what, wouldn't it be simpler to do so in one place, in the dialog's implementation at the same time you emit confirmImageUpload? That way it's impossible to forget, and much less likely to be broken by future changes.

Ralith · 2017-12-24T20:37:16Z

src/TextInputWidget.cc

+                        &FilteredTextEdit::receiveImage);
+                previewDialog_->show();
+        } else if (source->hasFormat("x-special/gnome-copied-files") &&
+                   QImageReader{source->text()}.canRead()) {


What if a user pastes in non-image files?

As of now, pasting non-image files will trigger the else clause and therefore, Qt's default behaviour will take over. I think adding support for pasting non-image files (generic files, aka not audio, video, etc) is fairly simple, but probably out of the scope of this PR. It is definitely something I can look into in the future.

Ralith · 2017-12-24T20:40:39Z

src/TextInputWidget.cc

+                previewDialog_->close();
+                return;
+        }
+        if (!img.save(file->fileName(), "PNG")) {


Saving the file to disk remains an unnecessary pessimization, and it's further unclear how you are guaranteeing that the temporary files are deleted when no longer needed.

You can encode images without touching the disk using a QImageWriter (for example).

Ralith · 2017-12-24T20:41:42Z

src/TextInputWidget.cc

+{
+        if (source->hasImage()) {
+                QPixmap const img = qvariant_cast<QPixmap>(source->imageData());
+                previewDialog_    = new dialogs::PreviewImageOverlay{img, this};


When does the PreviewImageOverlay you create here get deleted? Why does it need to be dynamically allocated at all?

The reason it is dynamically allocated is because the dialog is created on a paste event, which is dynamic behaviour by nature. The same approach is used in src/SideBarActions.cc and src/UserInfoWidget.cc, to name a few examples.

How it is deleted is an oversight. I have locally changed previewDialog_ to be a QSharedPointer so that it can be de-allocated automatically. I also pass this as a parameter to the constructor as to create a parent-child relationship with Qt's object model.

Please let me know if you have a better approach.

The UI semantics you describe are orthogonal to the actual construction/destruction of the object; you could let PreviewImageOverlay be a regular class member, and just show/hide/reset it as necessary. Dynamic allocation can be simpler, but requires care to avoid introducing leaks. Here, using Qt's parent-child relationship does not avoid a leak because many pastes may occur in the lifetime of a TextInputWidget.

Switching to a QSharedPointer fixes the problem, but may be confusing, as it's presumably not intended that ownership of the value be shared. Instead, I'd suggest using std::unique_ptr, and furthermore replacing the call to new with std::make_unique; calling new explicitly should generally be avoided, as it's prone to this type of mistake.

Ralith · 2017-12-24T20:42:22Z

src/TextInputWidget.cc

+                   QImageReader{source->text()}.canRead()) {
+                // Special case for X11 users. See "Notes for X11 Users" in source.
+                // Source: http://doc.qt.io/qt-5/qclipboard.html
+                previewDialog_ = new dialogs::PreviewImageOverlay{source->text(), this};


See above re: deletion.

Ralith · 2017-12-24T20:44:11Z

src/TextInputWidget.cc

+FilteredTextEdit::insertFromMimeData(const QMimeData *source)
+{
+        if (source->hasImage()) {
+                QPixmap const img = qvariant_cast<QPixmap>(source->imageData());


QMimeData::imageData is documented as returning a QVariant containing a QImage. Why is a QPixmap used here instead?

Ralith · 2017-12-24T20:48:29Z

src/MatrixClient.cc

@@ -711,16 +711,16 @@ MatrixClient::uploadImage(const QString &roomid, const QString &filename)
                        return;
                }

-                emit imageUploaded(roomid, filename, object.value("content_uri").toString());
+                emit imageUploaded(roomid, iodev.dynamicCast<QFile>(), object.value("content_uri").toString());


It's unclear whether there's any handling for the case where this cast fails and returns an empty QSharedPointer. Regardless, as discussed elsewhere, QFile should not be used here.

Ralith · 2017-12-24T20:49:57Z

src/timeline/widgets/ImageItem.cc

@@ -61,11 +61,11 @@ ImageItem::ImageItem(QSharedPointer<MatrixClient> client,

 ImageItem::ImageItem(QSharedPointer<MatrixClient> client,
                     const QString &url,
-                     const QString &filename,
+                     QSharedPointer<QFile> file,


How will this handle images that aren't written to disk?

I am unsure how to approach this yet as I did not foresee this as a problem.

To properly load the image, we also need access to the image data. It's not a difficult change, but requires numerous functions to be modified to be able to pass the image data along with the filename. I would like to hear your thoughts on this one.

Can you give a few examples of functions that would need to be modified? An image in the process of being uploaded shouldn't need to be accessed from very many places, and I don't imagine there are many places other than the timeline or the upload procedure itself where a filename is pertinent.

The following is the calling order which would need to be modified to pass the image data:

MatrixClient::imageUploaded

TimelineViewManager::queueImageMessage

TimelineView::addUserMessage

ImageItem constructor

Edit: The reason is because in item 4, a call to setImage is used with the image file name. If that file doesn't exist on the filesystem, then the issue occurs. The obvious solution is to pass the image data to setImage instead.

That doesn't seem particularly egregious to me. If the increase in number of parameters bothers you, you could always create a struct Image { QImage data; QString name; }; or similar to bundle them up for convenience.

christarazi · 2017-12-31T00:40:18Z

I've added a new commit which hopefully addresses all the issues pointed out.

Ralith

Thanks! This is shaping up nicely, but I think we can still make it even better.

Ralith · 2018-01-01T05:17:37Z

src/TextInputWidget.cc

+
+                const auto formats = source->formats();
+                const auto idx     = formats.indexOf(
+                  QRegularExpression{"image/\\w+", QRegularExpression::CaseInsensitiveOption});


Is there a reason to be more selective than image/.*?

Not necessarily. I just find it more reassuring than one that gives image/ as a potential match (even though that should never happen). I do not mind changing it if you prefer.

Mainly I'm leery of inadvertently filtering out future valid image types. It's not immediately obvious to me that Qt's interpretation of \w is a superset of valid media type characters. I wouldn't object to image/.+ either, but if something's generating clipboard contents with garbage mime-types anything we do is going to be best-effort anyway.

Oops I forgot to address this one. I will make the change locally, and add it to the next set of changes if there is one.

Ralith · 2018-01-01T05:21:07Z

src/TextInputWidget.cc

+FilteredTextEdit::insertFromMimeData(const QMimeData *source)
+{
+        if (source->hasImage()) {
+                const QImage image = qvariant_cast<QImage>(source->imageData());


On further investigation, it's apparent to me that we can and therefore must use QMimeData::data to avoid re-encoding the image at all, rather than decoding the image here and re-encoding it below. This will avoid silent quality loss, improve robustness, and reduce CPU and memory use. The mime type obtained from QMimeData should also be propagated alongside the data so it can be included in the matrix event verbatim, removing any need to guess it.

I apologize for not identifying this option earlier; I'm not super familiar with how rich copy/paste works.

Forgive me if I'm missing something here, but using QMimeData::data won't prevent re-encoding, because we must pass a QString representing the MIME type, which is then used to create (encode the raw pixels) the QByteArray. I also am not too familiar with copy/pasting media, but I've noticed that when copying an image (not from a file manager), it will copy the raw pixel bytes, understandably, as there's no notion of a file. I only came to this realization when creating this PR.

Therefore, re-encoding cannot be avoided. However, your suggestion has reduced the amount of code necessary and made it much cleaner.

we must pass a QString representing the MIME type, which is then used to create (encode the raw pixels) the QByteArray

My understanding is that QMimeData::data performs no transcoding. The supplied mimeType is used to select which of multiple pieces of attached data you want (from the alternatives exposed by formats). This can be observed in the QMimeDataPrivate::retrieveTypedData implementation's handling of the type == QVariant::ByteArray case, which for image data falls through ultimately to QMimeDataPrivate::getData, which is a simple linear search through dataList.

I also am not too familiar with copy/pasting media, but I've noticed that when copying an image (not from a file manager), it will copy the raw pixel bytes, understandably, as there's no notion of a file.

What was your methodology here? If we're genuinely receiving image/* mime types, as the current PR suggests, I would expect the data to be in the advertised format. In fact, reviewing the implementation, it appears to me that QMimeData::imageData returns nonempty results only when the mime type is application/x-qt-image. PNG data certainly doesn't have to be physically stored in a file to be valid.

Right clicking on a jpg image in Chrome, selecting "Copy image," and inspecting the clipboard state with xclip -o -selection clipboard -t TARGETS reveals that the clipboard contains data in formats image/png and text/html. Dumping the former yields a valid png image, and the latter yields a brief HTML document containing an image tag. I'm a bit perplexed by Chrome's apparent decision to transcode the image, but it's apparent that we do not need to.

re-encoding cannot be avoided

If uncompressed data is actually the norm, we should take care to encode it as png (or perhaps something more modern, like webp?) for sanity's sake, but per the above testing this does not appear to be the case.

Huh, seems I have drawn an incorrect conclusion because of a misinterpretation of the QImage::bits method. These are the steps that I took which led me to make my initial claim:

QImage img = qvariant_cast<QImage>(source->imageData()); QByteArray arr = QByteArray{img.bits(), img.byteCount()};

For some reason, I thought that this would essentially convert the QImage into a byte array. Instead, it gives the actual raw pixel bytes, which led me to incorrectly believe that "Copy image" copies raw bytes.

Alright well, I apologize for that, my knowledge of the clipboard is clearly lacking. I was not even aware of that particular capability of xclip, that's pretty nifty. Thanks for your informative reply.

I think QImage probably is uncompressed data, since it supports editing. I'm still a bit confused about whether imageData is ever nonempty when pasting e.g. image/png; it seems like it can't be, but I assume you've the original code and it does something at least some of the time. Perhaps Qt automatically decodes incoming pastes for convenience of display.

One potential subtlety to bear in mind is the case where many image formats available. For example, here's the list firefox supplies:

text/html text/_moz_htmlinfo text/_moz_htmlcontext image/png image/bmp image/x-bmp image/x-MS-bmp image/jpeg image/tiff image/x-icon image/x-ico image/x-win-bitmap image/vnd.microsoft.icon application/ico image/ico image/icon text/ico

Given that the data supplied by both firefox and chrome seems to be re-encoded (the input was a jpeg image, and the image/jpeg content was 6 times smaller than the input) IMO a lossless format should be preferred. I'm happy to work under the hypothesis that everyone's helpful and orders paste data in descending quality--i.e. that it is correct to pick the first image content if any exists--but if this is determined to often be false, we may in the future want to add some heuristics to try to make a good selection. Might be worth leaving a note to ourselves in a comment.

Ralith · 2018-01-01T05:27:25Z

include/timeline/TimelineView.h

@@ -51,13 +51,13 @@ struct PendingMessage
        PendingMessage(mtx::events::MessageType ty,
                       int txn_id,
                       QString body,
-                       QString filename,
+                       const QString &fn,


Why was this argument name changed?

Ralith · 2018-01-01T05:37:12Z

resources/styles/nheko-dark.qss

-    color: #caccd1;
-}
-
-QListWidget {


Why was this line deleted?

Oops that was accidental. It probably slipped in when I was doing a rebase onto upstream from my previous PR. It has been fixed. Thanks for catching this.

Ralith · 2018-01-01T05:38:22Z

include/timeline/widgets/FileItem.h

@@ -42,6 +42,7 @@ class FileItem : public QWidget

        FileItem(QSharedPointer<MatrixClient> client,
                 const QString &url,
+                 const QSharedPointer<QIODevice> data,


Why is this needed? Unlike with image/audio/video, I'm not sure we can/should do much in the client with the data of an arbitrary opaque file.

This is needed because TimelineView::addUserMessage takes in a generic type Widget, which happens to be one of the four FileItem, ImageItem, AudioItem, and VideoItem. Therefore, we need those types to have the same interface (parameters to the constructor).

I'm not particularly happy about having never-used arguments making things confusing on both ends for the sake of convenience to a template. That said, any refactoring of addUserMessage is probably best done separately from this PR.

Ralith · 2018-01-01T05:41:38Z

src/MatrixClient.cc

        request.setHeader(QNetworkRequest::ContentTypeHeader, mime.name());

-        auto reply = post(request, file.readAll());
+        auto reply = post(request, iodev->readAll());


It's inappropriate to read the QIODevice fully into memory before transmitting. This wastes time and memory, and could hang the UI. Instead, it should be streamed. Note that QNetworkAccessManager::post supports this natively.

I was not aware of that particular overload. Fixed.

I am having trouble with this one actually. It seems that QNetworkAccessManager::post when taking in a pointer of QIODevice, will try to create another QSharedPointer. This causes problems, because in our case, the pointer to QIODevice is already a QSharedPointer. Somewhere this causes a segfault because of a double-free.

I was able to reproduce the issue. The following code is based on the stack trace from gdb.

Here's the code:

#include <QBuffer> #include <QFile> #include <QPointer> #include <iostream> #include <memory> static void not_deleter(QObject *obj) { } static void deleter(QObject *obj) { delete obj; } auto foo(QIODevice *p) { std::cerr << "Before cast\n"; QBuffer *buf = qobject_cast<QBuffer*>(p); std::cerr << "After cast\n"; std::cerr << "Before unique_ptr ctor\n"; QSharedPointer<QIODevice>{p, &deleter}; // segfault QSharedPointer<QIODevice>{p, &not_deleter}; // OK std::cerr << "After unique_ptr ctor\n"; } int main() { std::cerr << "Begin main()\n"; QSharedPointer<QFile> ptr{new QFile{}}; std::cerr << "Before call to foo()\n"; foo(ptr.data()); std::cerr << "After call to foo()\n"; std::cerr << "End main()\n"; return 0; }

Here's the stack trace:

Thread 1 "nheko" received signal SIGSEGV, Segmentation fault. 0x0000000e00000001 in ?? () (gdb) bt #0 0x0000000e00000001 in () #1 0x00007ffff5ea1b29 in QMetaObject::cast(QObject const*) const () at /usr/lib/libQt5Core.so.5 #2 0x00007ffff5db9e7d in QNonContiguousByteDeviceFactory::createShared(QIODevice*) () at /usr/lib/libQt5Core.so.5 #3 0x00007ffff6b39ecc in () at /usr/lib/libQt5Network.so.5 #4 0x00007ffff6b40330 in () at /usr/lib/libQt5Network.so.5 #5 0x00007ffff6b41c0c in () at /usr/lib/libQt5Network.so.5 #6 0x00007ffff6b41cf5 in () at /usr/lib/libQt5Network.so.5 #7 0x00007ffff6bf4fda in () at /usr/lib/libQt5Network.so.5 #8 0x00007ffff5ec9452 in QObject::event(QEvent*) () at /usr/lib/libQt5Core.so.5 #9 0x00007ffff76d1e3c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5 #10 0x00007ffff76d9816 in QApplication::notify(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5 #11 0x00007ffff5e981e0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5 #12 0x00007ffff5e9ae46 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/libQt5Core.so.5 #13 0x00007ffff5ef50a4 in () at /usr/lib/libQt5Core.so.5 #14 0x00007ffff2670270 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0 #15 0x00007ffff2671f69 in () at /usr/lib/libglib-2.0.so.0 #16 0x00007ffff2671fae in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0 #17 0x00007ffff5ef4691 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5 #18 0x00007fffe8f2c152 in () at /usr/lib/libQt5XcbQpa.so.5 #19 0x00007ffff5e9682b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5 #20 0x00007ffff5e9fb18 in QCoreApplication::exec() () at /usr/lib/libQt5Core.so.5 #21 0x0000555555a21ee7 in main(int, char**) (argc=1, argv=0x7fffffffe028) at /home/chris/dev/nheko/src/main.cc:166

See the link above for the source code of QNonContiguousByteDeviceFactory::createShared.

Referring to the documentation of QSharedPointer::create, you can see that it does not create a new QSharePointer owning exactly the same object, but rather creates a new, unrelated QSharedPointer with its own, new, object, the arguments passed in being forwarded to the new object's constructor. This makes sense, because (although the weird syntax highlighting makes it a bit hard to see) the value being passed in is a pointer to the pointer, not the pointer itself.

The documentation for QNetworkAccessManager::post specifies that you must ensure the QIODevice remains valid until the returned reply emits its finished signal. If you're seeing memory errors, this may be the cause. I believe a convenient way to ensure the QIODevice lives that long would be attaching a closure that captures the QSharedPointer that owns it to the signal.

Ah, thanks. Yes that fixed the issue.

Ralith · 2018-01-01T05:49:16Z

src/TextInputWidget.cc

+void
+FilteredTextEdit::receiveImage(const QByteArray &img, const QString &img_name)
+{
+        QSharedPointer<QBuffer> buffer{new QBuffer{const_cast<QByteArray *>(&img), this}};


In addition to being const-unsafe (it is almost never correct to use const_cast), this exposes a pointer to the imageData_ member of PreviewImageOverlay. What happens if the user pastes in another image while the emitted QBuffer is still in use somewhere?

Instead, create a QBuffer that owns its own storage using its default constructor, and initialize it by calling QBuffer::setData.

Ralith · 2018-01-01T06:10:30Z

include/dialogs/PreviewImageOverlay.h

+        void setImageAndCreate(const QString &path);
+
+signals:
+        void confirmImageUpload(const QByteArray &data, const QString &img_name);


To avoid inadvertent sharing and possible UB, this should emit the QByteArray by value. QByteArray is internally ref-counted so doing so is cheap.

Ralith · 2018-01-01T06:10:52Z

include/dialogs/PreviewImageOverlay.h

+public:
+        PreviewImageOverlay(QWidget *parent = nullptr);
+
+        void setImageAndCreate(const QByteArray &data, const QString &type);


Because it copies it internally anyway, this might as well take the QByteArray by value.

alphapapa · 2018-01-01T06:35:17Z

@Ralith I'm just an interested user, but thanks for your very thorough reviews of this patch. I'm glad that nheko is getting such high-quality code! (I wish I had a reviewer like you for my own projects!)

christarazi · 2018-01-01T23:12:17Z

@Ralith I would like to echo @alphapapa 's comment. I must admit your attitude and determination for quality code is quite inspiring and motivating, and exhibits one of the many qualities that I love about free and open source software and their respective communities. So thank you for your hard work and I wish everyone a happy new year. :)

Ralith · 2018-01-02T01:06:59Z

Thank you both for saying so! I'm glad my comments have been helpful.

alphapapa · 2018-01-02T02:29:22Z

@christarazi Forgive me for neglecting to thank you for making this patch as well! :)

Ralith · 2018-01-03T17:18:54Z

src/dialogs/PreviewImageOverlay.cc

+PreviewImageOverlay::setImageAndCreate(const QByteArray &data, const QString &type)
+{
+        imageData_ = data;
+        image_.loadFromData(imageData_);


It would be nice to have a fallback (perhaps a QLabel with some explanatory text) for when this fails (and returns false), i.e. when the user pasted in an image of a type that Qt doesn't support decoding, or that QImage can't accurately guess the format of. In those cases we should still allow uploading, but the current behavior might produce a confusing UI.

Ralith · 2018-01-03T17:21:21Z

src/timeline/widgets/ImageItem.cc

@@ -83,7 +84,14 @@ ImageItem::ImageItem(QSharedPointer<MatrixClient> client,
        url_                 = QString("%1/_matrix/media/r0/download/%2")
                 .arg(client_.data()->getHomeServer().toString(), media_params);

-        setImage(QPixmap(filename));
+        if (!data.isNull()) {
+                data->reset();


The return code of this shouldn't be ignored unless you can prove that it's guaranteed to succeed.

Ralith · 2018-01-03T17:24:08Z

src/MatrixClient.cc

-        if (!file.open(QIODevice::ReadWrite)) {
-                qDebug() << "Error while reading" << filename;
+        if (!iodev->open(QIODevice::ReadWrite)) {
+                qDebug() << "Error while reading buffer" << iodev.data();


Logged errors should contain the specific error message.

Ralith · 2018-01-03T17:26:41Z

src/timeline/widgets/ImageItem.cc

                     const QString &filename,
                     QWidget *parent)
  : QWidget(parent)
  , url_{url}
-  , text_{QFileInfo(filename).fileName()}
+  , text_{QFileInfo{filename}.fileName()}


filename is no longer a path on disk, so this won't work. You probably want to just copy in filename directly.

mujx · 2018-01-07T09:54:17Z

Could you remove the full path from the default name on the modal and only set the name of the file?

christarazi · 2018-01-07T21:40:31Z

New commit added addressing the second round of issues. Thanks again for your vigilance and patience!

mujx · 2018-01-08T07:17:27Z

Thanks again for your work! Could you rebase on master to resolve the conflicts and let the ci run?

Squashes the following commits: - Use QKeySequence::Copy instead of hardcoded ctrl-c - Add ability to embed images and text in same message - Remove unneeded explicit call to insertFromMimeData() - Fix case for X11 users when copying an image - Add preview dialog for pasted images

- Image data no longer written to disk; only in memory - Fix incorrect type used in qvariant_cast - PreviewImageOverlay is no longer dynamic; created on stack - Images are encoded based on MIME data; default is PNG - Handle images that are not on disk in ImageItem - Close PreviewImageOverlay dialog after upload button click - Follow consistent dialog styles in PreviewImageOverlay

- Grab encoded image data directly from clipboard rather than explicity transcoding. - Fix inadvertant deletion in styles - Pass pointer to QNetworkAccessManager::post instead of reading all the data at once - Remove unnecessary use of const_cast - Use pass-by-value for QByteArray - Log reason for QIODevice failing to open - Add missing check of return value for QIODevice::reset - Do not create QFileInfo on nonexistent file - Add note about MIME image data - Only show filename inside PreviewImageOverlay rather than path - Add error message to PreviewImageOverlay on failed read

- Make regex for image type more lenient - Fix inadvertent renaming of parameter

christarazi · 2018-01-09T04:30:03Z

I have rebased the changes onto master.

Ralith

Good progress! Thanks for sticking with this.

Ralith · 2018-01-09T05:58:21Z

src/timeline/widgets/FileItem.cc

                   const QString &filename,
                   QWidget *parent)
  : QWidget(parent)
  , url_{url}
-  , text_{QFileInfo(filename).fileName()}
+  , text_{QFileInfo{filename}.fileName()}


filename no longer identifies a path on disk.

Ralith · 2018-01-09T05:58:42Z

src/timeline/widgets/VideoItem.cc

@@ -66,13 +66,15 @@ VideoItem::VideoItem(QSharedPointer<MatrixClient> client,

 VideoItem::VideoItem(QSharedPointer<MatrixClient> client,
                     const QString &url,
+                     const QSharedPointer<QIODevice> data,
                     const QString &filename,
                     QWidget *parent)
  : QWidget(parent)
  , url_{url}
  , text_{QFileInfo(filename).fileName()}


filename no longer identifies a path on disk.

For VideoItem, AudioItem, and FileItem, filename does actually identify a path on disk. Only ImageItem's doesn't. This is because they are only created by the user explicitly selecting them to upload.

Once they can be uploaded through other means (copy-paste), then I think it would make sense to modify this (similar to how ImageItem is now).

If they need a uniform interface, then the interface should be semantically uniform. That means no inconsistent special meanings. filename should be standardized as purely UI.

I'm also not a fan of this but it could be handled in a different PR.

I also don't like it, but I thought it would be out of the scope of this PR to add changes unrelated to image pasting. I can definitely tackle this after this PR is merged.

This PR changes the pervasive filename argument to file-ish widget constructors from "always a full path on disk" to not. IMO is is therefore in scope; inconsistency here is a landmine.

I merged it before addressing the filename issue because I didn't want to drag this PR any further with refactoring. Although it is related, I think it would be better to have a more focused PR without the fear of moving too much out of scope.

Ralith · 2018-01-09T06:03:28Z

src/timeline/widgets/ImageItem.cc

+        if (!null && reset) {
+                p.loadFromData(data->readAll());
+        } else {
+                p.load(filename);


Because filename no longer identifies a path on disk, this fallback doesn't make sense. An error message (see QIODevice::errorString) should be displayed instead.

Ralith · 2018-01-09T06:03:56Z

src/timeline/widgets/AudioItem.cc

                     const QString &filename,
                     QWidget *parent)
  : QWidget(parent)
  , url_{url}
-  , text_{QFileInfo(filename).fileName()}
+  , text_{QFileInfo{filename}.fileName()}


filename no longer identifies a path on disk.

Ralith · 2018-01-09T06:06:38Z

src/TextInputWidget.cc

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


Isn't hasText redundant to checks made by QTextEdit::canInsertFromMimeData?

Ralith · 2018-01-09T06:11:06Z

src/MatrixClient.cc


        QNetworkRequest request(QString(endpoint.toEncoded()));
-        request.setHeader(QNetworkRequest::ContentLengthHeader, file.size());
+        request.setHeader(QNetworkRequest::ContentLengthHeader, iodev->size());


Per the QIODevice::size documentation, this is incorrect if the device is not a random access device. The header should be omitted entirely if QIODevice::isSequential returns true.

The spec doesn't seem to require the header at all, so I'm not sure why we're providing it in the first place. I believe it works fine without, so maybe it would be better to just remove this line entirely? Or is it of use to middleboxes sometimes?

Ralith · 2018-01-09T06:16:24Z

src/MatrixClient.cc

-        if (!file.open(QIODevice::ReadWrite)) {
-                qDebug() << "Error while reading" << filename;
+        if (!iodev->open(QIODevice::ReadOnly)) {
+                qDebug() << "Error while reading buffer" << iodev->errorString();


A QIODevice is not necessarily a buffer.

- Fix incorrect fallback to filename - Fix incorrect error message - Remove unneeded http content length header - Remove redundant check for MIME data - Run make lint

mujx · 2018-01-09T18:41:24Z

include/timeline/TimelineView.h

@@ -59,7 +59,8 @@ struct PendingMessage
          , filename(filename)
          , event_id(event_id)
          , widget(widget)
-        {}
+        {


I think those were fine. There are some issues with the clang-format running on travis (macos) and the one on liinux, so this will fail on travis. Not sure if this behaviour can be changed with a flag.

OK I will revert these lines.

christarazi · 2018-01-10T16:34:49Z

I will begin work immediately on addressing the filename issue as discussed. Thanks for merging!

alphapapa · 2018-01-11T05:50:17Z

@christarazi Thanks for your work on this! I just pulled and built and tested, and it worked great! Using Ubuntu 14.04 with KDE 4, I took a screenshot with KSnapshot, copied it to the clipboard, and pasted it directly into nheko.

@mujx @Ralith Thanks for your work on this as well!

christarazi mentioned this pull request Dec 24, 2017

Implement #132: Support pasting images into room #138

Closed

Ralith suggested changes Dec 24, 2017

View reviewed changes

Ralith suggested changes Jan 1, 2018

View reviewed changes

Ralith reviewed Jan 3, 2018

View reviewed changes

christarazi added 6 commits January 8, 2018 20:26

Use generic QIODevice for uploading media

1f7d238

Use new QIODevice api for pasted images

47a27ca

Address the issues pointed out in review (3)

59f8b57

- Make regex for image type more lenient - Fix inadvertent renaming of parameter

christarazi force-pushed the iodev-pasteimg branch from 2fca709 to 59f8b57 Compare January 9, 2018 04:28

Ralith suggested changes Jan 9, 2018

View reviewed changes

Address the issues pointed out in review (4)

9b49b7f

- Fix incorrect fallback to filename - Fix incorrect error message - Remove unneeded http content length header - Remove redundant check for MIME data - Run make lint

mujx reviewed Jan 9, 2018

View reviewed changes

Revert unnecessary changes from make lint

f503e1f

mujx approved these changes Jan 10, 2018

View reviewed changes

mujx merged commit ddfce13 into mujx:master Jan 10, 2018

christarazi deleted the iodev-pasteimg branch January 10, 2018 21:39

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

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

Conversation

christarazi commented Dec 24, 2017

mujx commented Dec 24, 2017

Ralith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi commented Dec 31, 2017 • edited Loading

Ralith left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Ralith Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

christarazi Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Ralith Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

Ralith Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

Ralith Jan 1, 2018 • edited Loading

Choose a reason for hiding this comment

alphapapa commented Jan 1, 2018

christarazi commented Jan 1, 2018

Ralith commented Jan 2, 2018

alphapapa commented Jan 2, 2018

Ralith Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

mujx commented Jan 7, 2018

christarazi commented Jan 7, 2018

christarazi Dec 29, 2017 •

edited

Loading

christarazi Dec 30, 2017 •

edited

Loading

christarazi commented Dec 31, 2017 •

edited

Loading

Ralith left a comment •

edited

Loading

christarazi Jan 3, 2018 •

edited

Loading

christarazi Jan 1, 2018 •

edited

Loading

Ralith Jan 2, 2018 •

edited

Loading

christarazi Jan 3, 2018 •

edited

Loading

Ralith Jan 3, 2018 •

edited

Loading

christarazi Jan 3, 2018 •

edited

Loading

Ralith Jan 2, 2018 •

edited

Loading

Ralith Jan 1, 2018 •

edited

Loading

Ralith Jan 7, 2018 •

edited

Loading

Ralith Jan 1, 2018 •

edited

Loading

Ralith Jan 1, 2018 •

edited

Loading

Ralith Jan 3, 2018 •

edited

Loading

Ralith Jan 3, 2018 •

edited

Loading

Ralith Jan 9, 2018 •

edited

Loading