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

book: Migrate to glib-build-tools #1024

Merged
merged 4 commits into from Oct 20, 2022
Merged

Conversation

RealKC
Copy link
Contributor

@RealKC RealKC commented May 7, 2022

No description provided.

@RealKC RealKC requested a review from Hofer-Julian as a code owner May 7, 2022 11:28
@RealKC
Copy link
Contributor Author

RealKC commented May 7, 2022

I'm not sure what's causing the CI failures

@bilelmoussaoui
Copy link
Member

I'm not sure what's causing the CI failures

#1025 (comment)

Copy link
Collaborator

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Thanks!
But I guess this crate will be added to crates.io?
I'd rather wait for that.

@RealKC
Copy link
Contributor Author

RealKC commented May 7, 2022

That's fine with me

@Hofer-Julian
Copy link
Collaborator

Then let's wait for its release, update Cargo.toml and merge then :)

@RealKC RealKC force-pushed the glibbuildtools branch 3 times, most recently from d3b4130 to 749a282 Compare October 19, 2022 19:09
@RealKC
Copy link
Contributor Author

RealKC commented Oct 19, 2022

Alright, should be rebased on master now, and I fixed a broken link that snuck in. CI failed on my first push but that didn't seem to be related to my changes?

@Hofer-Julian
Copy link
Collaborator

Thanks, could you please also update the relevant section in the book where we add the gtk build dependency?

@RealKC
Copy link
Contributor Author

RealKC commented Oct 20, 2022

That's done now

@Hofer-Julian
Copy link
Collaborator

That's done now

Thanks, I will later fix the build failures on your branch and merge it.

obj.setup_settings();
obj.setup_actions();
obj.bind_settings();
self.instance().setup_settings();
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest calling self.instance() only once and store it in an obj / window variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why though? Is there a performance penalty?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there should be any, cc @sdroege for confirming that. But it doesn't make much sense to repeat the same call if you can store the obj once and re-use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure

let incremented_number = self.number.get() + 1;
// If `number` reached `MAX_NUMBER`,
// emit "max-number-reached" signal and set `number` back to 0
if incremented_number == MAX_NUMBER {
button.emit_by_name::<()>("max-number-reached", &[&incremented_number]);
button.set_property("number", &0);
self.instance()
Copy link
Member

Choose a reason for hiding this comment

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

Same as elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants