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.

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

Jump to bottom

Port set-window-scroll-bars #1535

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Port set-window-scroll-bars #1535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
91ef1eb
Port set-window-scroll-bars
Apr 27, 2019
c7dfa70
Revert "Port set-window-scroll-bars"
Aug 18, 2019
3f4da36
Revert "Revert "Port set-window-scroll-bars""
Aug 19, 2019
3bffffa
Merge branch 'master' into 1459
agraven Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 51 additions & 2 deletions rust_src/src/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ use crate::{
run_window_configuration_change_hook as run_window_conf_change_hook,
save_excursion_restore, save_excursion_save, select_window,
selected_window as current_window, set_buffer_internal, set_window_fringes,
update_mode_lines, window_list_1, window_menu_bar_p, window_scroll, window_tool_bar_p,
windows_or_buffers_changed, wset_redisplay,
set_window_scroll_bars, update_mode_lines, window_list_1, window_menu_bar_p, window_scroll,
window_tool_bar_p, windows_or_buffers_changed, wset_redisplay,
},
remacs_sys::{face_id, glyph_matrix, glyph_row, pvec_type, vertical_scroll_bar_type},
remacs_sys::{EmacsDouble, EmacsInt, Lisp_Type, Lisp_Window},
Expand Down Expand Up @@ -2034,6 +2034,55 @@ pub fn window_pixel_height_before_size_change(window: LispWindowValidOrSelected)
window.pixel_height_before_size_change
}

/// Set width and type of scroll bars of window WINDOW.
/// WINDOW must be a live window and defaults to the selected one.
///
/// Second parameter WIDTH specifies the pixel width for the vertical scroll
/// bar. If WIDTH is nil, use the scroll bar width of WINDOW's frame.
/// Third parameter VERTICAL-TYPE specifies the type of the vertical scroll
/// bar: left, right, nil or t where nil means to not display a vertical
/// scroll bar on WINDOW and t means to use WINDOW frame's vertical scroll
/// bar type.
///
/// Fourth parameter HEIGHT specifies the pixel height for the horizontal
/// scroll bar. If HEIGHT is nil, use the scroll bar height of WINDOW's
/// frame. Fifth parameter HORIZONTAL-TYPE specifies the type of the
/// horizontal scroll bar: bottom, nil, or t where nil means to not display
/// a horizontal scroll bar on WINDOW and t means to use WINDOW frame's
/// horizontal scroll bar type.
///
/// Return t if scroll bars were actually changed and nil otherwise.
#[lisp_fn(
name = "set-window-scroll-bars",
c_name = "set_window_scroll_bars",
min = "1"
)]
pub fn set_window_scroll_bars_lisp(
window: LispWindowLiveOrSelected,
width: LispObject,
vertical_type: LispObject,
height: LispObject,
horizontal_type: LispObject,
) -> bool {
let mut window: LispWindowRef = window.into();
let updated_window = unsafe {
Comment on lines +2067 to +2068
Copy link
Contributor

@denis631 denis631 Feb 16, 2020 •
edited

Choose a reason for hiding this comment

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

@clample In the original implementation, the window is accessed via decode_live_window which checks for the selected window being nil first, if yes -> use it instead of the passed window, otherwise check the passed window for being live (otherwise throwing a "wrong type argument exception") and then use it.

Do you think this may be the case here? Seems like semantics is not preserved.
reference: window.c line:264

Copy link
Collaborator

@shaleh shaleh Feb 17, 2020 •
edited

Choose a reason for hiding this comment

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

@denis631 LispWindowLiveOrSelected is meant to encapsulate the logic of decode_live_window.
Line 2067 gets the correct Window.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh so the #lisp_fn macro does all the necessary transformations? It this case mapping LispObject to LispWindowLiveOrSelected? I see. Nice. Sorry for false negative

Copy link
Collaborator

@shaleh shaleh Feb 17, 2020 •
edited

Choose a reason for hiding this comment

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

@denis631 correct. The lisp_fn macro is a proc macro which creates code that looks like:

pub fn new_defun(obj1: LispObject, obj2: LispObject, .......) -> LispObject {
    let result = original_defun(obj1.into(), obj2.into(), ....);
    result.into()
}

Which is super simple. All of the error handling, checking, and the like occurs in the From implementations. The lisp_fn macro gets more complicated when the lisp function is of variable arguments. These collect the args into a Vector.

Our convention is that where a Lisp function takes optional parameters we use the Option type. In the C code this is represented with a Nil value. Essentially C's LispObject is really an Option<LispValue>.

set_window_scroll_bars(
window.as_mut(),
width,
vertical_type,
height,
horizontal_type,
)
};

Copy link
Collaborator

@shaleh shaleh Aug 13, 2019 •
edited

Choose a reason for hiding this comment

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

Perhaps flip the following logic? I am not sure which I prefer.

if updated_window.is_null() {
    false
} else {
    unsafe { apply_window_adjustment(updated_window) };
    true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, this is more readable to me than !updated_window.is_null().

I updated the PR

if updated_window.is_null() {
false
} else {
unsafe { apply_window_adjustment(updated_window) };
true
}
}

include!(concat!(env!("OUT_DIR"), "/windows_exports.rs"));

/// Run `window-configuration-change-hook' for FRAME.
Expand Down
36 changes: 1 addition & 35 deletions src/window.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ static void select_window_1 (Lisp_Object, bool);

static struct window *set_window_margins (struct window *, Lisp_Object,
Lisp_Object);
static struct window *set_window_scroll_bars (struct window *, Lisp_Object,
Lisp_Object, Lisp_Object,
Lisp_Object);

void wset_window_parameters (struct window *, Lisp_Object);
void wset_update_mode_line (struct window *);
Expand Down Expand Up @@ -5676,7 +5673,7 @@ set_window_fringes (struct window *w, Lisp_Object left_width,
Scroll bars
***********************************************************************/

static struct window *
struct window *
set_window_scroll_bars (struct window *w, Lisp_Object width,
Lisp_Object vertical_type, Lisp_Object height,
Lisp_Object horizontal_type)
Expand Down Expand Up @@ -5744,36 +5741,6 @@ set_window_scroll_bars (struct window *w, Lisp_Object width,
return changed ? w : NULL;
}

DEFUN ("set-window-scroll-bars", Fset_window_scroll_bars,
Sset_window_scroll_bars, 1, 5, 0,
doc: /* Set width and type of scroll bars of window WINDOW.
WINDOW must be a live window and defaults to the selected one.

Second parameter WIDTH specifies the pixel width for the vertical scroll
bar. If WIDTH is nil, use the scroll bar width of WINDOW's frame.
Third parameter VERTICAL-TYPE specifies the type of the vertical scroll
bar: left, right, nil or t where nil means to not display a vertical
scroll bar on WINDOW and t means to use WINDOW frame's vertical scroll
bar type.

Fourth parameter HEIGHT specifies the pixel height for the horizontal
scroll bar. If HEIGHT is nil, use the scroll bar height of WINDOW's
frame. Fifth parameter HORIZONTAL-TYPE specifies the type of the
horizontal scroll bar: bottom, nil, or t where nil means to not display
a horizontal scroll bar on WINDOW and t means to use WINDOW frame's
horizontal scroll bar type.

Return t if scroll bars were actually changed and nil otherwise. */)
(Lisp_Object window, Lisp_Object width, Lisp_Object vertical_type,
Lisp_Object height, Lisp_Object horizontal_type)
{
struct window *w
= set_window_scroll_bars (decode_live_window (window),
width, vertical_type, height, horizontal_type);
return w ? (apply_window_adjustment (w), Qt) : Qnil;
}


DEFUN ("window-scroll-bars", Fwindow_scroll_bars, Swindow_scroll_bars,
0, 1, 0,
doc: /* Get width and type of scroll bars of window WINDOW.
Expand Down Expand Up @@ -6176,7 +6143,6 @@ displayed after a scrolling operation to be somewhat inaccurate. */);
defsubr (&Sset_window_configuration);
defsubr (&Scurrent_window_configuration);
defsubr (&Sset_window_margins);
defsubr (&Sset_window_scroll_bars);
defsubr (&Swindow_scroll_bars);
defsubr (&Swindow_vscroll);
defsubr (&Sset_window_vscroll);
Expand Down
3 changes: 3 additions & 0 deletions src/window.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,6 +1127,9 @@ extern Lisp_Object select_window (Lisp_Object window, Lisp_Object norecord,
bool inhibit_point_swap);
extern struct window *set_window_fringes (struct window *w, Lisp_Object left_width,
Lisp_Object right_width, Lisp_Object outside_margins);
extern struct window *set_window_scroll_bars (struct window *w, Lisp_Object width,
Lisp_Object vertical_type, Lisp_Object height,
Lisp_Object horizontal_type);
extern void apply_window_adjustment (struct window *);
extern void run_window_configuration_change_hook (struct frame *);

Expand Down
18 changes: 18 additions & 0 deletions test/rust_src/src/windows-tests.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,24 @@
(ert-deftest window-pixel-height-before-size-change ()
(window-pixel-height-before-size-change))

(ert-deftest set-window-scroll-bars ()
(let ((w1 (selected-window))
(w2 (split-window))
(columns (if (window-system) 1 0)))
(set-window-scroll-bars w1 1 t nil nil)
(should (equal `(1 ,columns t nil 0 nil) (window-scroll-bars w1)))
(should (equal `(1 ,columns t nil 0 nil) (window-scroll-bars nil)))
(set-window-scroll-bars w1)
(should (equal '(nil 0 nil nil 0 nil) (window-scroll-bars w1)))
(set-window-scroll-bars w2 1 t nil nil)
(should (equal `(1 ,columns t nil 0 nil) (window-scroll-bars w2)))

;; invoking set-window-scroll-bars with the same params would
;; not change the scroll bars and should return nil
(should (set-window-scroll-bars w1 1 t nil nil))
(should-not (set-window-scroll-bars w1 1 t nil nil))
(should (set-window-scroll-bars w1 2 t nil nil))))

(ert-deftest run-window-configuration-change-hook ()
(setq window-conf-change-hook-val 0)
(add-hook 'window-configuration-change-hook
Expand Down