Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FileDialog style and other efficiency fixes #634

Merged
merged 2 commits into from May 18, 2021

Conversation

HanYoung-uwu
Copy link
Contributor

Setting windows style on non-windows platform makes FileDialog looks hideous.
Default style
After set the Windows style

Note that "Windows" style is not what you usually want even if you like windows style. "QWindowsVistaStyle" is what people usually choose.

Other changes include:

  • Avoid extra copy on KiwixApp::openZimFile
  • Remove close() on QFile because it close file in destructor, call it here you pay one more check on destructing (and extra line of code)

@kelson42 kelson42 requested a review from mgautierfr May 3, 2021 17:39
@kelson42
Copy link
Collaborator

kelson42 commented May 3, 2021

I see no visible change on Ubuntu, I guess this impacts only Windows users.

@HanYoung-uwu
Copy link
Contributor Author

I see no visible change on Ubuntu, I guess this impacts only Windows users.

Well, this does impact me who is on Arch/KDE.

@HanYoung-uwu
Copy link
Contributor Author

I see no visible change on Ubuntu, I guess this impacts only Windows users.

Is your filedialog like the second picture above?

@kelson42
Copy link
Collaborator

@HanYoung-uwu This is how it looks like for me
image

@HanYoung-uwu
Copy link
Contributor Author

@HanYoung-uwu This is how it looks like for me
image

So in your case, QFileDialog opens native filedialog using XDG, your native filedialog is gnome so Qt style doesn't affect you. But for system which native filedialog is written in Qt, ugly windows theme is used.

Copy link
Member

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes on the git commits.
Noting to say on the code itself.


Remove close() on QFile because it close file in destructor, call it here you pay one more check on destructing (and extra line of code)

This comment could be put in the commit message itself
("Don't close the QFile explicitly as it will be close anyway in the destructor.")

src/kiwixapp.cpp Outdated
Comment on lines 160 to 168
getMainWindow(),
gt("open-zim"),
QString(),
"ZimFile (*.zim*)");

if (_zimfile.isEmpty()) {
return;
}
_zimfile = QDir::toNativeSeparators(_zimfile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be made in previous commit.

@HanYoung-uwu
Copy link
Contributor Author

force pushed. Please re-review

@mgautierfr
Copy link
Member

We are good. Thanks !

@mgautierfr mgautierfr merged commit c6594af into kiwix:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants