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

Delete unused QObjectBox, update kefia example #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ratijas
Copy link
Collaborator

@ratijas ratijas commented Jul 15, 2021

Goodbye, you have to go. We've never been close friends anyway.

Kefia example got a fresh look, although still using Quick Controls 1.x.
I unironically had to find ancient ruins of Yaourt repo, build and
install it, just to check the format of its -Qe output, because fields
were wrongly named and that got me confused. Expac replacement is
changes behavior to list all packages from all repos instead of only
locally installed ones, but that's something not trivial to workaround,
and its better than having forever-obsolete and forgotten dependency.

[ChangeLog][Breaking Change] QObjectBox was removed.

Fixes #170


Update: almost forgot the screenshot! Dark theme looks bad with Controls 1.x, so switched to the light for a second.
kefia

Goodbye, you have to go. We've never been close friends anyway.

Kefia example got a fresh look, although still using Quick Controls 1.x.
I unironically had to find ancient ruins of Yaourt repo, build and
install it, just to check the format of its -Qe output, because fields
were wrongly named and that got me confused. Expac replacement is
changes behavior to list all packages from all repos instead of only
locally installed ones, but that's something not trivial to workaround,
and its better than having forever-obsolete and forgotten dependency.

[ChangeLog][Breaking Change] QObjectBox was removed.

Fixes woboq#170
@ratijas
Copy link
Collaborator Author

ratijas commented Jul 15, 2021

Now just need to figure out how all these things supposed to work together, and revise object pinning.

trait QObject {}
trait QGadget {}
trait QEnum {}
struct QPointer;
struct QObjectRefMut;
struct QObjectPinned;

@ratijas ratijas requested a review from ogoffart July 15, 2021 11:56
@ogoffart
Copy link
Member

That's great!

However, removing QObjectBox would break semver, and i'd like to avoid doing that unless we have a good reason.
Perhaps it is enough to mark it as #[deprecated]

@ratijas
Copy link
Collaborator Author

ratijas commented Jul 15, 2021

According to Rust view of semver, 0.x minor versions don't have to be compatible, so 0.3 would be a breaking change anyway.

@ratijas ratijas added A-docs Area: documentation for any part of the projects A-rust Area: Rust glue C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Jul 15, 2021
@ratijas
Copy link
Collaborator Author

ratijas commented Jul 15, 2021

By the way, kefia would be better using Rust wrappers around libalpm directly. https://crates.io/crates/alpm

Maybe we should fork it completely: git-subtree copy into examples directory. That would ensure better support and up-to-date refactoring in the long run. Otherwise, I feel like I'm the only person who had patience to apply those patches.

@ogoffart
Copy link
Member

According to Rust view of semver, 0.x minor versions don't have to be compatible, so 0.3 would be a breaking change anyway

Yes, but I would hope that the next version would be 0.2.x
Because otherwise people have to update their Cargo.toml and stuff.

Maybe we should fork it completely

Feel free to do that. But then i guess it'd be another repository. The point of these patch stuff was just a proof of concept to show that it is possible. But I don't want to maintain that.

@ratijas
Copy link
Collaborator Author

ratijas commented Jul 16, 2021

But I don't want to maintain that.

I mean, not maintaining it like a useful piece of software with features and stuff. (Who needs pacman frontend anyway? Especially when KDE Discover is already doing good.) But rather just keeping it up-to-date with current code base of this framework, so that a proof of concept still holds water. It could be a separate repository, for sure. But then we'll have to link it in some new dedicated examples/showcases section somewhere in README — which is not a bad thing btw, just non-existent at the moment.

Yes, but I would hope that the next version would be 0.2.x

There is an option to create a 0.2 branch at the current commit, and cherry-pick critical hot fixes there. There is a re-export trick which would keep 0.2 compatible with types from 0.3 — the one they used with libc bindings. And anyway re-implementing object pinning will be a breaking change someday.

@ogoffart
Copy link
Member

There is an option to create a 0.2 branch at the current commit, and cherry-pick critical hot fixes there

Or we can just keep this QObjectBox as deprecated and remove it when we have reason to break semver.

@ratijas ratijas added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 20, 2021
@ratijas
Copy link
Collaborator Author

ratijas commented Jul 20, 2021

Let's stall this until pinning is revised then. It will sure bring enough reasons to break semver anyway. Otherwise, I just don't see the point — in this particular case — to keep it around for compatibility which is unlikely being used by any application code. There is no rush whatsoever.

@ratijas ratijas added this to the std::pin::Pin milestone Jul 20, 2021
@sztomi
Copy link
Contributor

sztomi commented Jul 23, 2021

If QObjectBox is being removed it would be nice to get an example of how to use std::Pin instead. My use-case is setting a global property that points to QObject that I want to access from qml:

  let dispatcher = QObjectBox::new(Dispatcher::default());
  engine.set_object_property("dispatcher".into(), dispatcher.pinned());

@ratijas
Copy link
Collaborator Author

ratijas commented Jul 23, 2021

If QObjectBox is being removed it would be nice to get an example of how to use std::Pin instead. My use-case is setting a global property that points to QObject that I want to access from qml:

  let dispatcher = QObjectBox::new(Dispatcher::default());
  engine.set_object_property("dispatcher".into(), dispatcher.pinned());

I think, patch to kefia number 5 answers your request. That was the only usage of QObjectBox in this repo, and your case looks exactly like it. Unfortunately, there is an unsafe block involved, but I hope new API would resolve that too.

@sztomi
Copy link
Contributor

sztomi commented Jul 24, 2021

I see, thanks. Honestly I think it would be better to keep this than delete it without a safe alternative in the API. The fact that it's only used in one of the examples is not necessarily representative of its usefulness overall. If it's removed I will probably copy the old QObjectBox into my code.

@rubdos
Copy link
Contributor

rubdos commented Dec 28, 2021

FYI, I have started some work that also gets rid of QObjectPinned in favour of std::pin::Pin. Seems to work so far. Just leaving this comment such that it does not go to waste if I don't manage to finish it.

First commits on that branch are a merge of master and the work in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the projects A-rust Area: Rust glue C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QObjectBox struct is redundant?
4 participants