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.

Sign up for GitHub

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 select-frame #1384

Open

yshalenyk wants to merge 3 commits into remacs:master from yshalenyk:select-frame

Contributor

yshalenyk commented Mar 14, 2019

No description provided.

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

Collaborator

shaleh commented Mar 14, 2019

Would you like to give porting do_switch_frame a go?

Collaborator

shaleh commented Mar 14, 2019

Typically, we would port frame as a LispFrameRef. In this case, it would immediately be cast back into a LispObject to call the C function do_switch_frame. So there is little value. However, if the C function is also ported then the Rust types have more usefulness.

Contributor Author

yshalenyk commented Mar 15, 2019

ok, I will try to do it.

yshalenyk changed the title ~~Port select-frame~~ [WIP] Port select-frame

brotzeit previously approved these changes

View reviewed changes

Member

brotzeit left a comment

Let's merge this. You can always open another PR.

Contributor Author

yshalenyk commented Apr 2, 2019

Sorry for the delay, folks. I've added an initial implementation of do_switch_frame, it's still incomplete but I think a little review would help ;)

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

shaleh changed the title ~~[WIP] Port select-frame~~ Port select-frame

shaleh reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

Contributor Author

yshalenyk commented Apr 9, 2019

I wonder how to deal with Travis builds errors. Basically, it's complaining about unused track argument, which is not used without "window-system" feature.

Collaborator

shaleh commented Apr 9, 2019

This means you do not have enough cfg protection. Which leads to track being exposed when it is not needed/used.

Collaborator

agraven commented Apr 13, 2019

I would recommend at least for now to just rename track to _track.

brotzeit force-pushed the select-frame branch from 9803172 to 2173797 Compare

May 5, 2019 09:38

brotzeit dismissed their stale review

May 5, 2019 09:42

needs another review

brotzeit reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

brotzeit reviewed

View reviewed changes

rust_src/src/frames.rs Outdated Show resolved Hide resolved

Collaborator

shaleh commented Jan 10, 2020

@yshalenyk are you interested in picking this back up?


Port select-frame

3f212f4

yshalenyk force-pushed the select-frame branch from 2173797 to 3f212f4 Compare

January 21, 2020 23:03

Contributor Author

yshalenyk commented Jan 22, 2020

@shaleh yes, I've rebased on top of current master and tweaked code based on review comments. CI failed I will fix it this evening


Fix frame_ancestor_p available only with window-system feature

7ef0d13

agraven reviewed

View reviewed changes

Collaborator

agraven left a comment

This looks like a gnarly one, thanks for taking a stab at it. I have a few stylistic suggestions, I have yet to properly look over the logic, but what I have looked at seems solid.

rust_src/src/frame.rs Outdated Show resolved Hide resolved

rust_src/src/frame.rs Outdated Show resolved Hide resolved

rust_src/src/frame.rs Outdated Show resolved Hide resolved

rust_src/src/frame.rs Outdated Show resolved Hide resolved


Improve rust style in do_switch_frame

e53a30d

agraven reviewed

View reviewed changes

rust_src/src/frame.rs

+ Some(f) if f.eq(&current_frame_ref) => unsafe {
+ Fredirect_frame_focus(xfocus, frame);
+ },
+ None if focus.is_nil()

Collaborator

agraven Jan 25, 2020 •

edited

focus.is_nil() should not* be necessary as it's implied by as_frame() returning None

*Misspelled not as now

rust_src/src/frame.rs Outdated

@@ @@ -857,96 +857,103 @@ pub extern "C" fn do_switch_frame( @@
  // This used to check for a live frame, but apparently it's possible for
  // a switch-frame event to arrive after a frame is no longer live,
  // especially when deleting the initial frame during startup.
- if let Some(mut target_frame) = frame.as_live_frame() {
- if target_frame.eq(&current_frame_ref) {

Collaborator

agraven Jan 25, 2020

I think this one got lost in the translation to a match statement. It could be added back my adding a

    Some(target_frame) if target_frame.eq(&current_frame_ref) => return frame,

branch to the start of the match

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment