BUG: 343839
Details
- Reviewers
pino tcanabrava - Group Reviewers
KDE Edu - Commits
- R331:9685a13c6b95: Fix the list of file types in Kig 'Open File' dialog
- Choose "File -> Open..."
- 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.
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.
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.
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.
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.
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.
kig/kig.cpp | ||
---|---|---|
221 | I find this to ve a repeated pattern in your code: You should use QPointer if you are unsure that the pointer will be pointing to something as you can check .isNull() , |
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? |
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; but if that crashes the app because of dbus... not worth it. |
Use QScopedPointer, fix inability to open two files from the same directory one after another.
kig/kig.cpp | ||
---|---|---|
221 | There are some not so old examples. It might be hard to crash apps this way, but it seems feasible. |
Revert to the agreed version to prevent slashing the patch due to the non-related changes.