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

Port make-frame-visible. #1526

Closed
wants to merge 4 commits into from
Closed

Port make-frame-visible. #1526

wants to merge 4 commits into from

Conversation

rbartlensky
Copy link
Contributor

This PR hopefully ports make-frame-visible to Rust. I am not including make-frame-invisible into this PR as it I need to port many functions for that, making the PR too big to review. I am not sure how to create unit tests for this function.

@shaleh
Copy link
Collaborator

shaleh commented Aug 1, 2019

error: variable does not need to be mutable
1863   --> src/frames.rs:713:9
1864    |
1865713 |     let mut frame_ref = window_frame_live_or_selected(frame);
1866    |         ----^^^^^^^^^
1867    |         |
1868    |         help: remove this `mut`

if window.is_null() {
return;
}
loop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a loop, let's implement an Iterator for LispWindowRef that knows how to walk the next window attribute. Look at LispSymbolRef's iterator as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, you are right. I made this change in my latest commit.

shaleh
shaleh previously requested changes Aug 1, 2019
Copy link
Collaborator

@shaleh shaleh left a comment

Choose a reason for hiding this comment

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

Good start, let's lean into Rust a bit more.

@rbartlensky
Copy link
Contributor Author

error: variable does not need to be mutable
1863   --> src/frames.rs:713:9
1864    |
1865713 |     let mut frame_ref = window_frame_live_or_selected(frame);
1866    |         ----^^^^^^^^^
1867    |         |
1868    |         help: remove this `mut`

I also included a fix for this in my last commit, however, I am not sure it is the best way to handle it...

}
}
}
let frame_ref: LispFrameRef = frame.into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe if we did this at the top? Then there would not be a reason to repeat it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that the cfg block needs a mutable pointer in the call to x_make_window_visible. However, when that cfg block is not compiled, the mut of frame_ref is unnecessary. I have a new commit to solve this issue without calling into multiple times.

@rbartlensky
Copy link
Contributor Author

@shaleh I guess this is ready for another review. However, I am not sure why make check doesn't work...

@yellowsquid
Copy link

Resurrected this here (#1573).

@agraven
Copy link
Collaborator

agraven commented Aug 7, 2020

Closing since #1573 replaces this PR

@agraven agraven closed this Aug 7, 2020
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

4 participants