Fix the list of file types in Kig "Open File" dialog
ClosedPublic

Authored by yurchor on Dec 19 2018, 7:36 PM.

Details

Summary

BUG: 343839

Test Plan
  1. Choose "File -> Open..."
  2. Click on "Filter:" list. Supported file filters should be shown.

Diff Detail

Repository
R331 Kig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yurchor created this revision.Dec 19 2018, 7:36 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptDec 19 2018, 7:36 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
yurchor requested review of this revision.Dec 19 2018, 7:36 PM
apol added a subscriber: apol.Dec 20 2018, 1:08 PM

They should be shown anyway, you mean the filters arguments does nothing? Maybe that's the bug to chase.

In D17697#379940, @apol wrote:

They should be shown anyway, you mean the filters arguments does nothing? Maybe that's the bug to chase.

Sorry, but no. According to docs [1], QFileDialog::getOpenFileName accepts filter const QString &filter = QString() in the form "Images (*.png *.xpm *.jpg)", not a MIME type list as now. That's why this dialog shows "All files" only in the filter section now.

[1] http://doc.qt.io/qt-5/qfiledialog.html#getOpenFileName

Kig "Open File" dialog in the current git/master:

pino requested changes to this revision.Dec 20 2018, 1:31 PM
pino added a subscriber: pino.

QFileDialog does support filtering by MIME types: you have to manually create/set/execute the dialog on your own, instead of using the static helper methods.
Please make use of this, instead of nullifying the MIME types provided by the filters.

This revision now requires changes to proceed.Dec 20 2018, 1:31 PM
In D17697#379949, @pino wrote:

instead of nullifying the MIME types provided by the filters.

Please explain this part. Thanks in advance.

pino added a comment.Dec 20 2018, 1:45 PM
In D17697#379949, @pino wrote:

instead of nullifying the MIME types provided by the filters.

Please explain this part. Thanks in advance.

Filtering by MIME type is not the same as filtering by extension. MIME type detection is based on extension and content, which makes file identification much better.

In D17697#379954, @pino wrote:
In D17697#379949, @pino wrote:

instead of nullifying the MIME types provided by the filters.

Please explain this part. Thanks in advance.

Filtering by MIME type is not the same as filtering by extension. MIME type detection is based on extension and content, which makes file identification much better.

That code is almost literally borrowed from the official Qt example. It does exactly what you are saying + allows to open "All files" if the detection fails. But Ok, you hate me and I will rewrite it as you wish.

pino added a comment.Dec 20 2018, 1:53 PM
In D17697#379954, @pino wrote:
In D17697#379949, @pino wrote:

instead of nullifying the MIME types provided by the filters.

Please explain this part. Thanks in advance.

Filtering by MIME type is not the same as filtering by extension. MIME type detection is based on extension and content, which makes file identification much better.

That code is almost literally borrowed from the official Qt example. It does exactly what you are saying + allows to open "All files" if the detection fails.

Getting the extensions of the MIME types basically throws away the indentification done with MIME types itself. This for example means that conflicting extensions will not be solved by file content detection.
Also, remember that the examples are code like the rest of the applications, and they can have bugs as well...

But Ok, you hate me and I will rewrite it as you wish.

I don't hate you. If you propose code for review, than I give my opinion based on my experience, which (at least from a programming POV) is still wider than yours.

yurchor updated this revision to Diff 47888.Dec 20 2018, 1:58 PM

Rewrite the code to use MIME type list directly.

The new "Open File" dialog:

Anyone, please? :'(

cfeck added a subscriber: cfeck.Dec 23 2018, 2:37 AM

Expect less people to check code in the next two weeks, there is no need to rush.

aacid added a subscriber: aacid.Dec 23 2018, 10:50 AM

Expect less people to check code in the next two weeks, there is no need to rush.

Just to make it clear, i have time to review this, i'm just staying away since Yuri specifically said he didn't want me to review his changes.

Expect less people to check code in the next two weeks, there is no need to rush.

Just to make it clear, i have time to review this, i'm just staying away since Yuri specifically said he didn't want me to review his changes.

Sorry. I meant that you do not need to review if it hurts you. Thanks for your patience, anyway.

tcanabrava requested changes to this revision.Dec 24 2018, 1:03 PM
tcanabrava added a subscriber: tcanabrava.
tcanabrava added inline comments.
kig/kig.cpp
221

I find this to ve a repeated pattern in your code:
1 - QPointer<Something> = new QType()
2 - Does nothing that's required for a QPointer
3 - Explicitly delete the pointer.

You should use QPointer if you are unsure that the pointer will be pointing to something as you can check .isNull() ,
if you need to delete the thing later, try to use QScopedPointer if you really *need* it to be on the heap
if you don't need it to be on the heap, just add it to stack.
This QFileDialog can be on the stack, I belive.

This revision now requires changes to proceed.Dec 24 2018, 1:03 PM
yurchor added inline comments.Dec 24 2018, 1:15 PM
kig/kig.cpp
221

Alms for the poor... I'm not a professional system programmer (it's obvious from the code, I think).

This is the recommended approach to deal with such cases:

https://blogs.kde.org/node/3919

I just followed the pattern as you have observed.

I have tried to make it like this:

https://phabricator.kde.org/D17738

but it does not compile.

I tried to google "creating QObjects on the stack" but it does not give some fast results. Can you recommend something to read about this?

tcanabrava accepted this revision.Dec 24 2018, 2:29 PM
tcanabrava added inline comments.
kig/kig.cpp
221

I was aware of the blogpost you posted (and hoped not to see it again), It's from 2009 and I'm not sure it applies today, I need to test. but with that blogpost in mind, I would say "Meh, then it's good"

On the stack means that the variable is not a pointer,

QFileDialog dlg;
dlg.exec();

but if that crashes the app because of dbus... not worth it.

yurchor updated this revision to Diff 48131.Dec 24 2018, 2:34 PM

Use QScopedPointer, fix inability to open two files from the same directory one after another.

yurchor added inline comments.Dec 24 2018, 2:37 PM
kig/kig.cpp
221

There are some not so old examples. It might be hard to crash apps this way, but it seems feasible.

https://phabricator.kde.org/D7285

yurchor updated this revision to Diff 48132.Dec 24 2018, 3:02 PM

Revert to the agreed version to prevent slashing the patch due to the non-related changes.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 24 2018, 3:07 PM
This revision was automatically updated to reflect the committed changes.