search

Pelagicore / gdbus-codegen-glibmm

Code generator for C++ D-Bus stubs and proxies using Giomm/Glibmm
GNU Lesser General Public License v2.1
23 stars 25 forks source link

Move *MessageHelper classes from common to stub #59

Closed kursatkobya closed 5 years ago

kursatkobya commented 5 years ago

issues/55 has justifiable questions the necessity of having MessageHelper classes on the common. So as advices this commit moves it to stub.h. This also initiates TypeWrap classes move to proxy.h.

kursatkobya commented 5 years ago

W.I.P.

To be able to totally comply with current version. MessageHelper class should have moved outside of the namespaces. Currently it is api_braker.

Lets have a discussion whether to make it comply with current version, or any other way.

mardy commented 5 years ago

Moving the MessageHelper classes to the stub.h is technically an API break, but in practice it's very unlikely to be a problem, given that these classes are totally irrelevant for the proxy side. So, I'm fine with this move.

The TypeWrap class, on the other side, could be used on both sides (think of signal parameters: they are travelling in the opposite direction). So, I don't think we should move them.

kursatkobya commented 5 years ago

Okay so changes on TypeWrap class should be deferred then, fair enough. About MessageHelper, if we move it into namespaces as it is proposed in this PR; we need to be sure the users of this tool should update their code regarding this change.

As another approach we can put MessageHelper class into stub but outside of the namespace in which case users of the tool does not have to do anything for current time being.

martin-ejdestig commented 5 years ago

Put branch with my changes (rebased to latest master and put fixup commits with small explanation of what each one does in commit message) here:

https://github.com/martin-ejdestig/gdbus-codegen-glibmm/commits/pull_59_fixups

Will sync with @kursatkobya when he gets back on Monday.

martin-ejdestig commented 5 years ago

@kursatkobya I force pushed to your branch now. Maybe you can modify the commit message and see if there is anything else you want to modify?

kursatkobya commented 5 years ago

I think this MR is mature enough but surely it breaks API so i propose to add new version then merge this.

martin-ejdestig commented 5 years ago

Looking at how "Invocation" is actually used maybe it is too short of a name. I suggested it but maybe MethodInvocation is better? Could keep MessageHelper name, I guess, but I do not like the fact that concepts that already exist in GDBus, GDBusMessage and GDBusMethodInvocation, are mixed up a due to this. (It confused me a bit at least. :)

mardy commented 5 years ago

+1 for MethodInvocation

I suggest to create a next branch containing this and other API-breaking changes. Once it's stable we can rename our master branch to version_2.x and merge all these changes into master.

martin-ejdestig commented 5 years ago

"next_3.x" or something? Makes sense to have "3" in it, I think. :)

kursatkobya commented 5 years ago

@mardy, @martin-ejdestig now the request is to merge into next branch

kursatkobya commented 5 years ago

The only problem will be maintenance/rebasing the branch to be up to date with master i guess.

martin-ejdestig commented 5 years ago

Force pushed Invocation -> MethodInvocation and removed the type alias added for backwards compatibility (since we are not putting this on master).

martin-ejdestig commented 5 years ago

Yes, branching off version 2 and continuing with (potentially api breaking) development on master sounds like a better idea than a separate dev/next branch. :+1:

jonte commented 5 years ago

Yes, branching off version 2 and continuing with (potentially api breaking) development on master sounds like a better idea than a separate dev/next branch.

This is done now. I'd also like to merge this: https://github.com/Pelagicore/gdbus-codegen-glibmm/pull/70 before adding breaking changes, so that we get a separate executable installed for the breaking changes.

jonte commented 5 years ago

+1 for MethodInvocation

I suggest to create a next branch containing this and other API-breaking changes. Once it's stable we can rename our master branch to version_2.x and merge all these changes into master.

Hm. I didn't actually see your comment about the branching before implementing it. My impression in #69 is that the code is stable enough to be released. If we find any issues, we should pick them into the version_2.x branch as well. I hope you agree. Sorry for missing your comment!