Port run-window-configuration-change-hook (#1441) #1545

xorphitus · 2019-10-10T15:44:26Z

Hello,
I hope it closes issue #1441

I have 2 questions.

Do I have to port DEFVAR_LISP ("window-configuration-change-hook", ...) using defvar_lisp!?
I've deleted static from C source codes. Is it a correct idea?
- I want to call run_window_configuration_change_hook C function from Rust source code

shaleh · 2019-10-10T21:13:38Z

Do I have to port DEFVAR_LISP ("window-configuration-change-hook", ...) using defvar_lisp!?

correct

I've deleted static from C source codes. Is it a correct idea?
I want to call run_window_configuration_change_hook C function from Rust source code

also, correct.

shaleh · 2019-10-10T21:17:30Z

rust_src/src/windows.rs

+/// Run `window-configuration-change-hook' for FRAME.
+/// If FRAME is omitted or nil, it defaults to the selected frame.
+#[lisp_fn(min = "0")]
+pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {


Suggested change

pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {

pub fn run_window_configuration_change_hook(frame: LispFrameLiveOrSelected) {

Wherever you see decode_live_frame you can use LispFrameLiveOrSelected instead. Then in the function:
let frame_ref: LispFrameRef = frame.into(); Now you have a Rust LispFrameRef to use.

shaleh · 2019-10-10T21:17:50Z

rust_src/src/windows.rs

+/// If FRAME is omitted or nil, it defaults to the selected frame.
+#[lisp_fn(min = "0")]
+pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {
+    let f = frame.map_or(Qnil, |obj| obj);


This line is not needed, see above.

shaleh · 2019-10-10T21:18:15Z

rust_src/src/windows.rs

+#[lisp_fn(min = "0")]
+pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {
+    let f = frame.map_or(Qnil, |obj| obj);
+    unsafe { run_window_conf_change_hook(decode_live_frame(f)) };


Suggested change

unsafe { run_window_conf_change_hook(decode_live_frame(f)) };

unsafe { run_window_conf_change_hook(frame.into()) };

shaleh · 2019-10-10T21:18:57Z

rust_src/src/windows.rs

+pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {
+    let f = frame.map_or(Qnil, |obj| obj);
+    unsafe { run_window_conf_change_hook(decode_live_frame(f)) };
+    Qnil


No need for the naked Qnil return. The lisp_fn wrapper handles this in the cases where the function should always return Qnil.

shaleh · 2019-10-10T21:19:07Z

rust_src/src/windows.rs

+    Qnil
+}
+
+#[rustfmt::skip]


Why the skip?

rustfmt causes an error when I try make.

$ make : make[1]: Entering directory '/path/to/remacs/src' GEN globals.h thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:378:21 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace. /bin/sh: line 1: 1785984 Aborted (core dumped) ../lib-src/make-docfile -d . -g dispnew.o frame.o scroll.o xdisp.o menu.o xmenu.o window.o charset.o coding.o category.o ccl.o character.o chartab.o bidi.o cm.o term.o terminal.o xfaces.o xterm.o xfns.o xselect.o xrdb.o xsmfns.o xsettings.o gtkutil.o emacsgtkfixed.o dbusbind.o emacs.o keyboard.o macros.o keymap.o sysdep.o buffer.o filelock.o insdel.o minibuf.o fileio.o dired.o casefiddle.o indent.o search.o regex.o undo.o alloc.o data.o doc.o editfns.o callint.o eval.o fns.o font.o print.o lread.o syntax.o unexelf.o bytecode.o process.o gnutls.o callproc.o region-cache.o sound.o atimer.o doprnt.o intervals.o textprop.o composite.o xml.o lcms.o inotify.o profiler.o thread.o systhread.o sheap.o xfont.o ftfont.o xftfont.o ftxfont.o fontset.o fringe.o image.o xgselect.o json.o ../rust_src/src/*.rs > globals.tmp make[1]: *** No rule to make target 'gl-stamp', needed by 'globals.h'. Stop. make[1]: Leaving directory '/path/to/remacs/src' make: *** [Makefile:429: src] Error 2

And I followed these codes.
https://github.com/remacs/remacs/blob/master/rust_src/src/process.rs#L558-L559

rustfmt seems to break macro expansion.
Do you have any idea?

This code is formatted to following

def_lisp_sym!( Qwindow_configuration_change_hook, "window-configuration-change-hook" );

Can you show the backtrace you get when you reproduce the error with RUST_BACKTRACE=1 set?

Here's the backtrace.

$ RUST_BACKTRACE=1 make make -C lib all make[1]: Entering directory '/path/to/remacs/lib' make[1]: Nothing to be done for 'all'. make[1]: Leaving directory '/path/to/remacs/lib' make -C lib-src all make[1]: Entering directory '/path/to/remacs/lib-src' make[1]: Nothing to be done for 'all'. make[1]: Leaving directory '/path/to/remacs/lib-src' make -C src VCSWITNESS='$(srcdir)/../.git/logs/HEAD' all make[1]: Entering directory '/path/to/remacs/src' GEN globals.h thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/libcore/option.rs:378:21 stack backtrace: 0: backtrace::backtrace::libunwind::trace at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/libunwind.rs:88 1: backtrace::backtrace::trace_unsynchronized at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.29/src/backtrace/mod.rs:66 2: std::sys_common::backtrace::_print at src/libstd/sys_common/backtrace.rs:47 3: std::sys_common::backtrace::print at src/libstd/sys_common/backtrace.rs:36 4: std::panicking::default_hook::{{closure}} at src/libstd/panicking.rs:200 5: std::panicking::default_hook at src/libstd/panicking.rs:214 6: std::panicking::rust_panic_with_hook at src/libstd/panicking.rs:477 7: std::panicking::continue_panic_fmt at src/libstd/panicking.rs:384 8: rust_begin_unwind at src/libstd/panicking.rs:311 9: core::panicking::panic_fmt at src/libcore/panicking.rs:85 10: core::panicking::panic at src/libcore/panicking.rs:49 11: core::option::Option<T>::unwrap at /rustc/bc2e84ca0939b73fcf1768209044432f6a15c2e5/src/libcore/macros.rs:12 12: scan_rust_file at remacs-lib/docfile.rs:168 /bin/sh: line 1: 2857917 Aborted (core dumped) ../lib-src/make-docfile -d . -g dispnew.o frame.o scroll.o xdisp.o menu.o xmenu.o window.o charset.o coding.o category.o ccl.o character.o chartab.o bidi.o cm.o term.o terminal.o xfaces.o xterm.o xfns.o xselect.o xrdb.o xsmfns.o xsettings.o gtkutil.o emacsgtkfixed.o dbusbind.o emacs.o keyboard.o macros.o keymap.o sysdep.o buffer.o filelock.o insdel.o minibuf.o fileio.o dired.o casefiddle.o indent.o search.o regex.o undo.o alloc.o data.o doc.o editfns.o callint.o eval.o fns.o font.o print.o lread.o syntax.o unexelf.o bytecode.o process.o gnutls.o callproc.o region-cache.o sound.o atimer.o doprnt.o intervals.o textprop.o composite.o xml.o lcms.o inotify.o profiler.o thread.o systhread.o sheap.o xfont.o ftfont.o xftfont.o ftxfont.o fontset.o fringe.o image.o xgselect.o json.o ../rust_src/src/*.rs > globals.tmp make[1]: *** No rule to make target 'gl-stamp', needed by 'globals.h'. Stop. make[1]: Leaving directory '/path/to/remacs/src' make: *** [Makefile:429: src] Error 2

Ah, I see. When we parse the def_lisp_sym! macro in docfile.rs, we can only correctly parse it if it's all on one line. Same with defvar_lisp! and the others. For now I suggest leaving the #[rustfmt::skip] and we'll deal with docfile.rs separately.

@db48x Thank you!
You meant, this regexp doesn't support line breaks?
https://github.com/remacs/remacs/blob/master/rust_src/remacs-lib/docfile.rs#L166

-def_lisp_sym!$(.+?),\s+"(.+?)"$; +def_lisp_sym!$\s*(.+?),\s*"(.+?)"\s*$;

I found that defvar_lisp! has the same issue.

shaleh · 2019-10-10T21:19:48Z

src/window.h

@@ -1128,6 +1128,7 @@ extern Lisp_Object select_window (Lisp_Object window, Lisp_Object norecord,
 extern struct window *set_window_fringes (struct window *w, Lisp_Object left_width,
                                          Lisp_Object right_width, Lisp_Object outside_margins);
 extern void apply_window_adjustment (struct window *);
+extern void run_window_configuration_change_hook (struct frame *);


shaleh · 2019-10-10T21:20:37Z

test/rust_src/src/windows-tests.el

@@ -201,3 +201,14 @@
 ;; frames inside tests (see https://github.com/remacs/remacs/issues/1429).
 (ert-deftest window-pixel-height-before-size-change ()
  (window-pixel-height-before-size-change))
+


🎉 Tests 🎉

shaleh

Great first draft. The changes are minor.

xorphitus · 2019-10-11T08:29:47Z

I fixed my code's issues with this commit.
74916e7
If there is no issues, I'll fixup it for merge.

~~Sorry, CI failed.~~
~~Now I'm trying to fix it.~~

-> I did it.
92420a9

agraven

Excellent work, thanks for the PR

Requested changes addressed

Port run-window-configuration-change-hook (remacs#1441)

d0236dd

shaleh reviewed Oct 10, 2019

View reviewed changes

shaleh previously requested changes Oct 10, 2019

View reviewed changes

fixup: Port run-window-configuration-change-hook (remacs#1441)

74916e7

fixup: Port run-window-configuration-change-hook (remacs#1441)

92420a9

agraven approved these changes Jan 12, 2020

View reviewed changes

agraven merged commit e602302 into remacs:master Jan 12, 2020

xorphitus deleted the run-window-configuration-change-hook branch January 13, 2020 05:22

jdkschang pushed a commit to jdkschang/remacs that referenced this pull request Jun 24, 2020

Port run-window-configuration-change-hook (remacs#1441) (remacs#1545)

7c4db91

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port run-window-configuration-change-hook (#1441) #1545

Port run-window-configuration-change-hook (#1441) #1545

xorphitus commented Oct 10, 2019

shaleh commented Oct 10, 2019

shaleh Oct 10, 2019

shaleh Oct 10, 2019

shaleh Oct 10, 2019

shaleh Oct 10, 2019

shaleh Oct 10, 2019

xorphitus Oct 11, 2019 •

edited

agraven Oct 11, 2019

xorphitus Oct 12, 2019

db48x Oct 12, 2019

xorphitus Oct 13, 2019

shaleh Oct 10, 2019

shaleh Oct 10, 2019

shaleh left a comment

xorphitus commented Oct 11, 2019 •

edited

agraven left a comment

	pub fn run_window_configuration_change_hook(frame: Option<LispObject>) -> LispObject {
	pub fn run_window_configuration_change_hook(frame: LispFrameLiveOrSelected) {

	unsafe { run_window_conf_change_hook(decode_live_frame(f)) };
	unsafe { run_window_conf_change_hook(frame.into()) };

Port run-window-configuration-change-hook (#1441) #1545

Port run-window-configuration-change-hook (#1441) #1545

Conversation

xorphitus commented Oct 10, 2019

shaleh commented Oct 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xorphitus Oct 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaleh left a comment

Choose a reason for hiding this comment

xorphitus commented Oct 11, 2019 • edited

agraven left a comment

Choose a reason for hiding this comment

xorphitus Oct 11, 2019 •

edited

xorphitus commented Oct 11, 2019 •

edited