Port KImageMapEditor away from deprecated KUrl, saveWindowSize and initialGeometrySet()
ClosedPublic

Authored by yurchor on Sep 21 2018, 5:12 AM.

Details

Summary

The deprecated functions spoil compilation log. I have used convert-kurl.pl to get rid of KUrl, then some manual editing to fix the rough corners.

Test Plan
  1. The application compiles.
  2. Opening/Saving files seems to work for me.

Diff Detail

Repository
R443 KImageMapEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yurchor requested review of this revision.Sep 21 2018, 5:12 AM
yurchor created this revision.
aacid added a subscriber: aacid.Sep 24 2018, 9:13 AM
aacid added inline comments.
kimagemapeditor.cpp
168

While at it can you change this connect to new style syntax to make sure it's not breaking with the KUrl->QUrl change?

yurchor added inline comments.Sep 24 2018, 3:49 PM
kimagemapeditor.cpp
168

setPicture is overloaded (twice). Can we leave it as is without esotheric casting?

I have wasted an hour but cannot find an answer what to choose in overload of "connect( imagesListView, &ImagesListView::imageSelected, this, &KImageMapEditor::setPicture);"

Please advise on how to resolve this.

Thanks in advance.

yurchor marked 2 inline comments as done.Sep 25 2018, 3:20 PM

Ok. I have found the cast.

yurchor updated this revision to Diff 42311.Sep 25 2018, 3:23 PM

Add ugly connect in the "new" style.

aacid added inline comments.Sep 25 2018, 10:00 PM
kimagemapeditor.cpp
168

Sorry i didn't answer earlier, but QOverload<QUrl>::of(&KImageMapEditor::setPicture) should also work, it's a bit less ugly than the static_cast (may need to be const QUrl & instead of QUrl, not sure)

yurchor marked an inline comment as done.Sep 26 2018, 5:23 AM

Thanks. I have tried QOverload<QUrl>::of(&KImageMapEditor::setPicture) and it fails. So I began to search among the harder routes. QOverload<const QUrl &>::of(&KImageMapEditor::setPicture) works indeed.

yurchor updated this revision to Diff 42343.Sep 26 2018, 5:25 AM

The simpler way to get the "new" style connect working.

aacid added a comment.Oct 12 2018, 4:30 PM

Opening an image (Via File->Open) doesn't work (which i realize in the non ported version doesn't work either, but the error is different so you may want to have a look anyway.

I'm trying to open /home/tsdgeos/tsdgeos_bn.png while running kimagemapeditor in /home/tsdgeos/devel/kde/kimagemapeditor

The error i get is
"The image ./home/tsdgeos/tsdgeos_bn.png does not exist."
note the weird . in the beginning.

With the non patched version i get "The image /home/tsdgeos/tsdgeos_bn.png could not be opened." that is also a bug but at least the path is correct :D

Opening an image (Via File->Open) doesn't work (which i realize in the non ported version doesn't work either, but the error is different so you may want to have a look anyway.

Uh, File->Open works for me with the current master for both web pages and images (at least png and jpegs), and I remember that I tested it back then.

yurchor updated this revision to Diff 43527.Oct 13 2018, 9:02 AM

Carefully test all corner cases in the url handling. Remove double warning in the document rewrite.

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

This looks reasonable to me, but since Luigi is the one that did most of the last work i'll let him do the actual approval for the patch :)

Thanks for the work. A quick note: I'm watching this but I'm getting a crash in some cases when opening a file, it may be my environment so I need to investigate it more.

yurchor updated this revision to Diff 43788.Oct 17 2018, 1:06 PM

Thoroughly follow porting guide to avoid the inability to load files that were previously saved.

Sorry for not testing everything before proposing changes.

It seems to be mostly working, with one issue/regression: when you pass a file name from the command line, kimagemapeditor crashes:

#6  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#7  0x00007f8ff97452f1 in __GI_abort () at abort.c:79
#8  0x00007f8ff9c7d997 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007f8ff9c7ce19 in qt_assert(char const*, char const*, int) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x00007f8ffbbe4788 in KProtocolInfoFactory::findProtocol (this=0x7f8ffbcb27d0 <(anonymous namespace)::Q_QGS_kProtocolInfoFactoryInstance::innerFunction()::holder>, protocol=...) at /home/prova/kde-svn/git.kde.org/frameworks/kio/src/core/kprotocolinfofactory.cpp:72
#11 0x00007f8ffbbe3c97 in KProtocolInfo::protocolClass (_protocol=...) at /home/prova/kde-svn/git.kde.org/frameworks/kio/src/core/kprotocolinfo.cpp:331
#12 0x00007f8ffbfa9124 in KParts::ReadOnlyPart::openUrl (this=0x56315ab6a140, url=...) at /home/prova/kde-svn/git.kde.org/frameworks/kparts/src/readonlypart.cpp:154
#13 0x0000563159985e7c in KImageMapEditor::openURL(QUrl const&) ()
#14 0x0000563159985db8 in KImageMapEditor::openFile(QUrl const&) ()
#15 0x0000563159965ca9 in KimeShell::openFile(QUrl const&) ()
#16 0x0000563159964111 in main ()

If you pass the full URL to the file (file:///<full_path>), then it works. At least one KUrl->QUrl conversion should be revisited.

yurchor updated this revision to Diff 44097.Oct 23 2018, 9:09 AM

Parse command line correctly.

ltoscano accepted this revision as: ltoscano.Oct 23 2018, 9:54 PM

Thanks

This revision is now accepted and ready to land.Oct 23 2018, 9:54 PM
This revision was automatically updated to reflect the committed changes.