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

Partial properties generation #640

Merged
merged 2 commits into from Sep 28, 2018

Conversation

Projects
None yet
3 participants
@EPashkin
Copy link
Member

EPashkin commented Sep 25, 2018

Part of #639 and #510

cc @GuillaumeGomez, @sdroege

IMHO needed other name instead "object.property.generate", maybe for PropertyGenerateFlags too.

WIP as I don't like current parser implementation and try to rework it.

README.md Outdated
@@ -189,6 +189,12 @@ cfg_condition = "mycond"
name = "baseline-position"
version = "3.10"
ignore = true
[[object.property]]
name = "events"
# generate only `connect_property_notify_event`, without `get_property_events` and `set_property_events`

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Sep 25, 2018

Member

Shouldn't it be connect_property_notify_eventS?

This comment has been minimized.

@EPashkin

EPashkin Sep 25, 2018

Author Member

Strange, but is not:

    fn connect_property_notify_event<F: Fn(&Self, &gdk::EventProperty) -> Inhibit + 'static>(&self, f: F) -> SignalHandlerId {
        unsafe {
            let f: Box_<Box_<Fn(&Self, &gdk::EventProperty) -> Inhibit + 'static>> = Box_::new(Box_::new(f));
            connect(self.to_glib_none().0, "property-notify-event",
                transmute(property_notify_event_trampoline::<Self> as usize), Box_::into_raw(f) as *mut _)
        }
    }

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Sep 25, 2018

Member

Let's call it dark magic haha.

This comment has been minimized.

@EPashkin

EPashkin Sep 25, 2018

Author Member

You was right, I looked to wrong function: connect_property_events_notify

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Sep 25, 2018

Member

Ah now it seems more logical! (And explain why I couldn't find it...)

This comment has been minimized.

@sdroege

sdroege Sep 25, 2018

Member

IMHO needed other name instead "object.property.generate", maybe for PropertyGenerateFlags too.

generate seems fine to me

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Sep 25, 2018

Member

Oh sorry, forgot to answer this question: seems fine to me as well.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 25, 2018

Seems good to me. Thanks a lot for adding it!

@EPashkin EPashkin force-pushed the EPashkin:partial_properties branch from 4c64bc5 to 9df71d6 Sep 25, 2018

README.md Outdated
name = "events"
# generate only `connect_property_events_notify`, without `get_property_events` and `set_property_events`
# supported values: "get", "set", "notify",
# also supported union "get|set" and inverting "not(notify)"

This comment has been minimized.

@sdroege

sdroege Sep 25, 2018

Member

Why so complicated that we even need nom? :)

I would simply list the things that should be generated. What is the use-case of negations?

This comment has been minimized.

@EPashkin

EPashkin Sep 25, 2018

Author Member

For 3 only cases not really unusable, I was do it automatically.
Maybe better only support union with simple list splitting and parsing.
I also thought that we need other generate flags in future (for signal), but seems I mixed up with something.

This comment has been minimized.

@sdroege

sdroege Sep 26, 2018

Member

Yes better just have a list :)

@EPashkin EPashkin force-pushed the EPashkin:partial_properties branch from 9df71d6 to 8f0ca08 Sep 26, 2018

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 26, 2018

Changed expression parsing

README.md Outdated
name = "events"
# generate only `connect_property_events_notify`, without `get_property_events` and `set_property_events`
# supported values: "get", "set", "notify", also allowed union "get|set"
generate = "notify"

This comment has been minimized.

@sdroege

sdroege Sep 26, 2018

Member

Why not a toml list of strings?

This comment has been minimized.

@EPashkin

EPashkin Sep 26, 2018

Author Member

Then we need accept both array and single value: too much work on Toml types,
and only array IMHO redundantly.
Can change if @GuillaumeGomez prefer array too.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 26, 2018

Looks good to me otherwise

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 26, 2018

@GuillaumeGomez Which config format for flags you prefer:
generate="get|set" or generate=["get","set"]?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Sep 26, 2018

I have a preference for generate=["get","set"].

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 26, 2018

Ok, then I do it tomorrow

@EPashkin EPashkin force-pushed the EPashkin:partial_properties branch from 8f0ca08 to b17fb50 Sep 27, 2018

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 27, 2018

Updated

@EPashkin EPashkin changed the title WIP: Partial properties generation Partial properties generation Sep 27, 2018

let name_for_func = nameutil::signal_to_snake(&name);
let var_name = nameutil::mangle_keywords(&*name_for_func).into_owned();
let get_func_name = format!("get_property_{}", name_for_func);
let set_func_name = format!("set_property_{}", name_for_func);
let check_get_func_name = format!("get_{}", name_for_func);
let check_set_func_name = format!("set_{}", name_for_func);

let mut readable = prop.readable;
let mut readable = prop.readable && generate.contains(PropertyGenerateFlags::GET);

This comment has been minimized.

@sdroege

sdroege Sep 27, 2018

Member

It should probably warn if GET is configured but the property is write-only (and same for writable)

This comment has been minimized.

@EPashkin

EPashkin Sep 27, 2018

Author Member

Good idea

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 27, 2018

Seems good to me except for some cosmetic detail #640 (review)

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 27, 2018

Added commit with warning

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 27, 2018

Maybe it need be error for faster detection

@EPashkin EPashkin force-pushed the EPashkin:partial_properties branch from 7e9a488 to 702e550 Sep 27, 2018

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Sep 27, 2018

Fine with me either way :) Thanks!

@EPashkin

This comment has been minimized.

Copy link
Member Author

EPashkin commented Sep 28, 2018

Then I merge it

@EPashkin EPashkin merged commit 9790f3c into gtk-rs:master Sep 28, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@EPashkin EPashkin deleted the EPashkin:partial_properties branch Sep 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.