Port KolourPaint away from deprecated KFileDialog
AbandonedPublic

Authored by yurchor on Sep 8 2018, 5:49 PM.

Details

Reviewers
aacid
mkoller
Group Reviewers
KDE Applications
Summary

KFileDialog is deprecated in KF5, so it should be ported to get rid of KDELIBS4SUPPORT.

This patch is just a quick fix to discuss a way to create QFileDilaog with a custom widget. Is it possible that subclassing works better?

Test Plan

Press Ctrl+Shift+S.

The new dialog is not native but resembles the original one.

Diff Detail

Repository
R374 KolourPaint
Lint
Lint Skipped
Unit
Unit Tests Skipped
yurchor requested review of this revision.Sep 8 2018, 5:49 PM
yurchor created this revision.
aacid added inline comments.Sep 19 2018, 10:58 PM
mainWindow/kpMainWindow_File.cpp
889

Why not use the native dialog?

yurchor marked an inline comment as done.Sep 20 2018, 4:41 AM
yurchor added inline comments.
mainWindow/kpMainWindow_File.cpp
889

From Qt 5.11 documentation:

By default, a platform-native file dialog will be used if the platform has one. In that case, the widgets which would otherwise be used to construct the dialog will not be instantiated, so related accessors such as layout() and itemDelegate() will return null. You can set the DontUseNativeDialog option to ensure that the widget-based implementation will be used instead of the native dialog.

yurchor marked an inline comment as done.Sep 23 2018, 4:08 PM

- Current version of the "Save file" dialog
- Ported (non-native) dialog

aacid added a comment.Sep 23 2018, 9:10 PM

The ported dialog looks bad, both because the layout of the new widget is not where you would want it to be and because it has much less functionality than the native dialog.

It's an interesting problem though which i don't know how to fix. Maybe it could be a post save dialog? Like "ok, since now you chose to save as jpeg i'll ask you which quality you want to use"

Which seems to be like how gimp does it. What do you think?

The ported dialog looks bad, both because the layout of the new widget is not where you would want it to be and because it has much less functionality than the native dialog.

It's an interesting problem though which i don't know how to fix. Maybe it could be a post save dialog? Like "ok, since now you chose to save as jpeg i'll ask you which quality you want to use"

Which seems to be like how gimp does it. What do you think?

Sure. I will try to upload the patch asap.

yurchor updated this revision to Diff 43584.Oct 14 2018, 1:11 PM

Implementing additional "Save Options" dialog to avoid adding custom widgets into QFileDialog.

Just to be sure if I'm moving in an acceptable direction (resizing does not work for some reason yet). Screenshot:

Why can't we use the native KF5 one?

yurchor added a comment.EditedOct 14 2018, 3:43 PM

Why can't we use the native KF5 one?

See above. QFileDialog does not allow adding custom widgets (like KolourPaint's saveOptionsWidget) in the native mode.

I like the separate window you've implemented, that seems like a good approach and then we can use the standard dialog.

aacid added a comment.Oct 15 2018, 9:12 PM

Just to be sure if I'm moving in an acceptable direction (resizing does not work for some reason yet). Screenshot:

Looks almost good, but the spacings seem a bit off, the spinbox is too close to the top and then there's too much space between spinbox and the image itself.

yurchor added a comment.EditedOct 17 2018, 6:42 PM

Spacing can be easily fixed with lay->setMargin(10);.

But somehow I have messed up when merging this good old code from the option widget and preview window. I cannot figure up quickly what is wrong and why it is impossible to resize the picture label and rewrite the existing image. Sorry.

I would be very obliged if someone can just give a little hint on what I have done wrong. Thanks in advance for your possible help.

Meanwhile, I will try to find the bug by myself (it cannot be committed now as it does not work).

aacid added a comment.Oct 17 2018, 9:32 PM

Save as crashes here, can you have a look?

aacid added a comment.Oct 17 2018, 9:34 PM

Save as crashes here, can you have a look?

Ah, you need m_filePixmap = nullptr; in init

aacid added a comment.Oct 17 2018, 9:51 PM

Spacing can be easily fixed with lay->setMargin(10);.

But somehow I have messed up when merging this good old code from the option widget and preview window. I cannot figure up quickly what is wrong and why it is impossible to resize the picture label and rewrite the existing image. Sorry.

I would be very obliged if someone can just give a little hint on what I have done wrong. Thanks in advance for your possible help.

Meanwhile, I will try to find the bug by myself (it cannot be committed now as it does not work).

You need to remove Qt::AlignCenter from the m_filePixmapLabel addWidget call

aacid added a comment.Oct 17 2018, 9:57 PM

Some more comments:

  • I'm not sure a grid layout makes the most sense, for example when making the dialog very wide, it ends up with the label and the input box very far apart from eachother, maybe it would make sense to be just a vertical layout with horizontal layouts inside? (food for though)
  • If while i am at /home/tsdgeos/devel/kde/kolourpaint/build i run kolourpaint ~/bla.jpg and then press Ctrl+S it'll show me a dialog wanting me to save bla.bmp in /home/tsdgeos/devel/kde/kolourpaint/build That is a regression from what the current kolourpaint does (both the extension and the path)
  • I would add a Cancel button to the Save Options dialog, because at some point i want to press Alt+F4 and for me mentally Alt+F4 is "go away i don't know what to answer" which is more a cancel than anything, so it feels ultra weird when pressing Alt+F4 and it seems like it has continued saving (because it has). Do you think i make any sense or just being picky?
yurchor updated this revision to Diff 43879.Oct 18 2018, 4:28 PM

New version.

Fixed:

  • Flexible resize of the PixLabel (thanks for the hints).
  • Switched from QGridLayout to QHBoxLayout inside of QVBoxLayout.
  • Added "Cancel" button and allowed to cancel saving on the second step.

Known bugs:

  • Somehow without urls even touched I managed to break loading the settings. Now the "Recent URLs" item in kolourpaintrc is ignored by QFileDialog and startURL is always empty.
  • It is impossible to save data in the same file by clicking it in the "Save as" dialog now. Preview is empty and the image data are lost in the processing line somehow.
yurchor updated this revision to Diff 44376.Oct 28 2018, 6:00 PM

Remember and restore last save location (stolen from KTorrent).

mkoller requested changes to this revision.Oct 28 2018, 7:06 PM

What I see is that selecting "Save As..." opens without suggesting a file name and a format (empty file name, format shows "all supported files").
The original version suggested the original name and format.
Also, the combobox showing all supported formats in "Save as" does not add the file name suffix as is done in the "open" file dialog - which it seems
is the same as it was in the original version ... still inconsistent, which might be good to look into also now. The list should alway shows the filename extension.

This revision now requires changes to proceed.Oct 28 2018, 7:06 PM
yurchor updated this revision to Diff 44458.Oct 29 2018, 6:42 PM

Fix "Save as..." dialog and glitches with a selection of existing files.

The globs in save dialogs (afaik) cannot be added easily with the current scheme of the code interaction. @mkoller , as an author of askForOpenURLs rewrite, you might already know this. We may look at it, but simple solutions (MIME filter <-> File name filter) seem to be very fragile. Any suggestions?

yurchor updated this revision to Diff 44482.Oct 30 2018, 9:46 AM

Fix handling of new images when saving.

yurchor abandoned this revision.Nov 28 2018, 4:39 AM

Abandoned because in the distant future there might be better solution (or will be no KolourPaint at all).

or will be no KolourPaint at all

???

or will be no KolourPaint at all

???

Just a supposition based on the current situation.

aacid added a comment.Nov 28 2018, 6:50 PM

Yuri, you really need to stop with your trolling, this is like the third time i've asked you to be polite and you keep trolling with no reason.

Also why are you abandoning this? It seemed to be in relative good shape.

Also why are you abandoning this? It seemed to be in relative good shape.

I have lost the will to continue with KolourPaint. Nothing more, no trolling. It seems that we are trying to find some ideal solution and I just feel that my life is too short for this.

I have learned from the above discussion that this problem (with KFileDialog) and the other problem (with KMimeType) from 3 last unported things are closely related, maintainer knows about them and can solve them without my intrusion. For me, on the other hand, it all reminds the grook:

A bit beyond perception's reach
I sometimes believe I see
that KolourPaint is two locked boxes, each
containing the other's key.

;)

And once more, in short, I did not want to insult anyone. The problem is just too complex. I cannot solve it now in the current state. So I just do not want to waste your time anymore.

Sorry for the already wasted time, just think that I have lost more time than you if it makes it easier.

If some code from this patch can be reused somehow, anyone can commandeer this revision and finish the porting.

Useful link for the courageous:

https://git.reviewboard.kde.org/r/129929/

cfeck added a subscriber: cfeck.Nov 28 2018, 9:05 PM

Thanks Yuri for your work. You already did much more than what we can ask for from a translator.

When I had ported KolourPaint to KF5, I couldn't solve these issues, and left them as they are in the hope that underlying frameworks (Qt5 or KF5) re-gain the missing functionality. If the coming Qt6 port would still need stuff from KDELibs4Support, I am sure we find a way to port these to Qt6, so that we don't need to abandon it.