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

Add basic QSettings wrapper #285

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Conversation

direc85
Copy link
Contributor

@direc85 direc85 commented Apr 9, 2023

Upstream-compatible version of whisperfish#2


Attempt of adding QSettings wrapper. This was a bit tricky because QSettings can't be copied, so we have to return QSettings * instead, and handle the dereference in the client. This leads into having these in the use-side implementation, but there's no obvious way to get rid of that to me:

fn inner(&self) -> &QSettings {
    unsafe { self.inner_ptr.as_ref().unwrap() }
}

fn inner_mut(&mut self) -> &mut QSettings {
    unsafe { self.inner_ptr.as_mut().unwrap() }
}

@rubdos
Copy link
Contributor

rubdos commented Apr 10, 2023

@direc85
Copy link
Contributor Author

direc85 commented Apr 10, 2023

Interesting indeed. Something about Windows and tempfile, I guess. I have a Windows 'puter somewhere still, so I can see what's up with that.

@direc85 direc85 force-pushed the qsettings-upstream branch 2 times, most recently from a52f8ac to f3128a6 Compare May 28, 2023 16:11
@direc85
Copy link
Contributor Author

direc85 commented May 28, 2023

It turned out to be a combination of default file extension being .ini (on all platforms?) and Linux conveniently using IniFormat when set to NativeFormat.

I must also say that it would have been convenient to have all tests run to completion even if one test fails. Could that be changed in the future? Thanks...

///
/// [class]: https://doc.qt.io/qt-5/qsettings.html
#[derive(Default)]
pub unsafe struct QSettings as "QSettings"
Copy link
Member

Choose a reason for hiding this comment

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

QSettings is a QObject and i'm not confortable having that in a cpp_class that can be moved.

I think i would do pub unsafe struct QSettings as "std::unique_ptr<QSettings>"

Co-authored-by: Olivier Goffart <olivier.goffart@slint-ui.com>
@direc85
Copy link
Contributor Author

direc85 commented Oct 28, 2023

This just became a blocker to Whisperfish for using master now that we have Rust 1.72 - thanks to the marvelous efforts of @rubdos - so could this please be merged to master? Thanks!

@ogoffart ogoffart merged commit 1538101 into woboq:master Oct 30, 2023
@ogoffart
Copy link
Member

sorry, i didn't see the updated patch.

@direc85
Copy link
Contributor Author

direc85 commented Oct 30, 2023

No worries! Thank you!

@direc85 direc85 deleted the qsettings-upstream branch October 30, 2023 22:02
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.

3 participants