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

Generate Keymap bindings #311

Merged
merged 4 commits into from Oct 31, 2019
Merged

Conversation

@bilelmoussaoui
Copy link
Contributor

bilelmoussaoui commented Aug 14, 2019

Fixes #310

Missing functions:

  • get_entries_for_keycode
  • get_entries_for_keyval
  • lookup_key

The three depend on KeymapKey that causes an issue, see the comment below.

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Aug 14, 2019

Tried generating Gdk.KeymapKey too but I'm getting this

thread 'main' panicked at 'Missing memory management functions for Gdk.KeymapKey', src/codegen/record.rs:75:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:39
   1: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at src/libstd/sys_common/backtrace.rs:59
             at src/libstd/panicking.rs:197
   3: std::panicking::default_hook
             at src/libstd/panicking.rs:211
   4: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:474
   5: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:381
   6: std::panicking::begin_panic_fmt
             at src/libstd/panicking.rs:336
   7: libgir::codegen::record::generate
   8: libgir::file_saver::save_to_file
   9: libgir::codegen::records::generate
  10: libgir::codegen::generate
  11: gir::main
  12: std::rt::lang_start::{{closure}}
  13: std::panicking::try::do_call
             at src/libstd/rt.rs:49
             at src/libstd/panicking.rs:293
  14: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:85
  15: std::rt::lang_start_internal
             at src/libstd/panicking.rs:272
             at src/libstd/panic.rs:394
             at src/libstd/rt.rs:48
  16: main
  17: __libc_start_main
  18: _start
make: *** [Makefile:22: src/auto/mod.rs] Error 101
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Aug 14, 2019

Which means it needs to be done by hand. :)

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Aug 14, 2019

Oh, will do that :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Aug 14, 2019

Look at other similar integer-like structs in the manual bindings. It'll help you to understand how to do it.

@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Aug 14, 2019

Ready to be reviewed :)
Thanks for the tips!

src/auto/keymap_key.rs Outdated Show resolved Hide resolved
src/keys.rs Outdated Show resolved Hide resolved
@bilelmoussaoui bilelmoussaoui force-pushed the bilelmoussaoui:gen-keymap branch from 06e634a to bf7d9ec Aug 14, 2019

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
#[repr(C)]
pub struct KeymapKey {

This comment has been minimized.

Copy link
@sdroege

sdroege Aug 14, 2019

Member

Why not make it a tuple struct around ffi::GdkKeyMapKey? Then we don't have to duplicate the struct layout here

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

Still a question :)

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Aug 14, 2019

Otherwise looks good to me but CI is unhappy

error[E0412]: cannot find type `GdkKeymapKey` in this scope
  --> src\keymap_key.rs:53:54
   |
53 | impl FromGlibPtrFull<*mut gdk_sys::GdkKeymapKey> for GdkKeymapKey {
   |                                                      ^^^^^^^^^^^^
help: a struct with a similar name exists
   |
53 | impl FromGlibPtrFull<*mut gdk_sys::GdkKeymapKey> for KeymapKey {
   |                                                      ^^^^^^^^^
help: possible candidate is found in another module, you can import it into scope
   |
5  | use gdk_sys::GdkKeymapKey;
   |
error: Compilation failed, aborting rustdoc
@bilelmoussaoui bilelmoussaoui mentioned this pull request Sep 23, 2019
40 of 43 tasks complete
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 22, 2019

get_entries_for_keycode is badly generated, this is a gir bug. Working on it.

@GuillaumeGomez GuillaumeGomez force-pushed the bilelmoussaoui:gen-keymap branch from 8ea9320 to 3f2fc09 Oct 23, 2019
let mut r_keyvals = Vec::with_capacity(n_entries);
for i in 0..n_entries {
r_keys.push(from_glib_none(keys.add(i)));
r_keyvals.push(ptr::read(keyvals.add(i)));

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

This seems strange. Why (Vec<KeymapKey>, Vec<u32>) and not Vec<(KeymapKey, u32)> if they're the same size anyway and apparently belong together? :)

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Oct 31, 2019

Member

Just thought about it afterwards. :)

&mut keyvals,
n_entries.as_mut_ptr(),
));
if ret {

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

Why not return an empty Vec if there were no entries? Less wrapping and that's exactly what a return value of false signals: no entries.

}
}

pub fn get_entries_for_keyval(&self, keyval: u32) -> Option<Vec<KeymapKey>> {

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

Why not return an empty Vec if no entries? Same as above

use ModifierType;

#[doc(hidden)]
impl ToGlib for &mut ModifierType {

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

Why is this needed?

impl Keymap {
pub fn add_virtual_modifiers(&self, state: &mut ModifierType) {
unsafe {
gdk_sys::gdk_keymap_add_virtual_modifiers(self.to_glib_none().0, state.to_glib());

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

This function sets bits in the state, but here you pass a copy of the state to the C function and don't read it back afterwards.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Oct 31, 2019

Member

Based on this code: https://code.woboq.org/gtk/gtk/gdk/x11/gdkkeys-x11.c.html#gdk_x11_keymap_add_virtual_modifiers, it updates the state so I'm not sure what you're saying. It seems good to me?

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 31, 2019

Member
    pub fn add_virtual_modifiers(&self, state: &mut ModifierType) {
        unsafe {
            let mut s = state.to_glib();
            gdk_sys::gdk_keymap_add_virtual_modifiers(self.to_glib_none().0, &mut s);
            *state = from_glib(state);
       }
    }

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Oct 31, 2019

Member

Oh I see! Thanks!

type GlibType = *mut gdk_sys::GdkModifierType;

fn to_glib(&self) -> *mut gdk_sys::GdkModifierType {
&mut self.bits() as *mut _

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

This is wrong. You're casting some random integer to a pointer

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Oct 31, 2019

Member

How would you do it?

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 31, 2019

Member

Remove this :)

unsafe {
from_glib(gdk_sys::gdk_keymap_map_virtual_modifiers(
self.to_glib_none().0,
state.to_glib(),

This comment has been minimized.

Copy link
@sdroege

sdroege Oct 24, 2019

Member

Also wrong like add_virtual_modifiers() with regards to the state

@GuillaumeGomez GuillaumeGomez force-pushed the bilelmoussaoui:gen-keymap branch from 3f2fc09 to 733c0d4 Oct 31, 2019
@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 31, 2019

Updated.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Oct 31, 2019

👍

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Oct 31, 2019

Finally! \o/

@GuillaumeGomez GuillaumeGomez merged commit f512d2a into gtk-rs:master Oct 31, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bilelmoussaoui

This comment has been minimized.

Copy link
Contributor Author

bilelmoussaoui commented Oct 31, 2019

Thanks @GuillaumeGomez for taking care of this

@bilelmoussaoui bilelmoussaoui deleted the bilelmoussaoui:gen-keymap branch Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.