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

Ported Overlay Split View in Rust #559

Merged
merged 8 commits into from
Aug 30, 2023

Conversation

itsAdee
Copy link
Contributor

@itsAdee itsAdee commented Aug 30, 2023

Hey. I ported Overlay Split View in Rust.
Tried to follow the Java Script code, but there were ownership errors so i had to use ref cell library and borrow_mut() function to resolve the ownership issues for split view .
Please check this approach it works correctly on my machine and let me know on how can i make it better.

@itsAdee itsAdee requested a review from Hofer-Julian as a code owner August 30, 2023 06:34
Copy link
Contributor

@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.

Reading this chapter should clear some things up.
https://gtk-rs.org/gtk4-rs/stable/latest/book/g_object_memory_management.html

Comment on lines 7 to 9
let overlay_split_view: Rc<RefCell<adw::OverlaySplitView>> = Rc::new(RefCell::new(
workbench::builder().object("split_view").unwrap(),
));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should need neither Rc nor RefCell here.
GObjects already behave like Rc<RefCell<T>>

Comment on lines 14 to 15
let overlay_split_view_clone = overlay_split_view.clone();
start_toggle.connect_toggled(move |_| {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use glib::clone! here.

Comment on lines 21 to 22
let overlay_split_view_clone = overlay_split_view.clone();
end_toggle.connect_toggled(move |_| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also use glib::clone! here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also use glib::clone! here

Made the changes please check again, and thanks again for the memory management resource it helped a lot. :)

Copy link
Contributor

@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.

I've changed your code slightly.
You can find the changes here: 6ad7246

Copy link
Contributor

@saadulkh saadulkh left a comment

Choose a reason for hiding this comment

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

I've added some comments as well. @Hofer-Julian Can you confirm if there's any difference with and without the changes that I suggested..?

.set_sidebar_position(gtk::PackType::Start);
}));

end_toggle.connect_toggled(glib::clone!(@strong overlay_split_view =>move |_| {
Copy link
Contributor

Choose a reason for hiding this comment

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

clone! is not needed here.

let start_toggle: gtk::ToggleButton = workbench::builder().object("start_toggle").unwrap();
let end_toggle: gtk::ToggleButton = workbench::builder().object("end_toggle").unwrap();

start_toggle.connect_toggled(glib::clone!(@strong overlay_split_view => move |_| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use @weak reference to the overlay_split_view here.

@Hofer-Julian
Copy link
Contributor

I agree with @saadulkh suggestions

@itsAdee
Copy link
Contributor Author

itsAdee commented Aug 30, 2023

I agree with @saadulkh suggestions

Added them , thanks to both of you for your suggestions :)

@Hofer-Julian Hofer-Julian merged commit 12e15f9 into workbenchdev:main Aug 30, 2023
1 check passed
@itsAdee itsAdee deleted the OverlaySplitView branch September 25, 2023 05:14
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