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

backport from gnome-menus 3.13.3 #59

Merged
merged 7 commits into from May 27, 2018

Conversation

Projects
None yet
5 participants
@yetist
Copy link
Member

yetist commented Apr 23, 2018

mate-menus 1.21.0

This version of matemenu is backport from gnome-menus, so it brings
some features, but break the compatibility of API and ABI.

As of now, there is no guide to migrate to the new API, but it is rather
straight-forward as changes were mostly done to improve the experience for
introspection-based bindings. The examples shipped in util/ are a good basis.

libmenu
* Rebase internal representation of .desktop files on top of GDesktopAppInfo.
* Make MateMenuTree a GObject.
* Port to pygobject-based introspection bindings.
* Use thread-default main context for callbacks for future flexibility for thread support.
* Support XDG_CURRENT_DESKTOP.
* Return GIcon instead of char * for icon-related API.
* Drop support for "KDE Desktop Entry" group.
* Many API changes, see new headers for changes. The most visible one
is that matemenu_tree_load_sync() should explicitly be used to load the
menu now.

Menu Editor
* Add simple menu editor tool, use introspection-based bindings
* Make editor GTK+ 3 ready.
Python
* Drop static python bindings; introspection-based ones should be used now.
* Python3 support
Misc
* Rewrite python example code to use introspection-based binding.
* Add javascript example code to use introspection-based binding.
* Require gio-unix-2.0 2.50.0 for new API
* Require python 3.6 for new API

Translations
* Translations update
* New language added.

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 23, 2018

If this pull request is accepted, the code of the following software also needs to be modified accordingly.

  1. mate-control-center
  2. mate-panel
  3. mate-screensaver
  4. mozo
@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented Apr 24, 2018

I really like to see this in 1.21/22 as in next fedora (f29) python2 is droped.
But we need to push PRs for the other applications at the same time to master.
So those needs to be prepared first.
@monsta @flexiondotorg @lukefromdc
Do you agree and can you help with rewiewing
@vkareh
I think you're happy if use this PR instead of yours?

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 24, 2018

Yes, you are right. We should complete the modification of other software before the release of 1.22.
So I plan to take a look at mozo first and let it use this new API.

@yetist yetist referenced this pull request Apr 24, 2018

Closed

just reindent python code #38

@vkareh

This comment has been minimized.

Copy link
Member

vkareh commented Apr 24, 2018

I'm happy to drop my PR and use this instead, but this is a massive change! Since it's a backport, would it make sense to cherry-pick the relevant commits so that we can track against gnome-menus? (Can we even drop mate-menus and use gnome-menus instead?)

Also, we need to drop the translation files from the pull request.

If we go forward with this, we need to update not only the apps mentioned above by @yetist, but also:

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 25, 2018

Cherry-pick the relevant commits from gnome-menus to mate-menus one bye one is a hard work, because:

  1. mate-menus and gnome-menus do not have the same commit id in git repository as parent commit object, which leads them to be unable to merge directly. mate-menus droped the early commit history.
  2. mate-menus and gnome-menus have different function names, object names, and so on. There are many conflicts when every merge.

If gnome-menus is used instead of mate-menus, there may be no problem at runtime, but gnome-common is needed at compile time.

If we want the third party software to run OK, maybe we can upgrading the API version of mate-menus, such as mate-menus-2 or mate-menus-3?

@lukefromdc

This comment has been minimized.

Copy link
Member

lukefromdc commented Apr 25, 2018

One issue for me is I know very little about Python compared to working in C. Thus I can evaluate results but could easily miss code issues

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented Apr 25, 2018

Can we even drop mate-menus and use gnome-menus instead?

It seems that gnome-menus doesn't pull in any non-wanted dependency if you install it, which is good.
But is it maintained?
Last commits are from 2016 and another 2 from 2014 from gnome-flashback team in their git history.
And last release is 3.13.3.
Sounds like it isn't used by gnome-shell.

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented Apr 25, 2018

Maybe MenuLibre?

Menulibre use already gnome-menus.
https://src.fedoraproject.org/cgit/rpms/menulibre.git/tree/menulibre.spec#n20

Maybe mintmenu?

Job by linuxmint, btw. they can use commits from mate-menu.

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 25, 2018

menulibre looks better, but only one thing is that can menulibre reorder the menu or menu items shows in the menu?

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented Apr 25, 2018

I am not familar with menulibre.
And it isn't from MATE.
So, why it is related to this PR or discussion about using mate- or gnome-menus?

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 26, 2018

Now mozo and mate-screensaver is ready to use this new api, but need more review and test, and mate-panel will be ok soon, now compile passed.
I think we can let them all ready before release mate 1.22.

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented Apr 27, 2018

Mozo, mate-screensaver, mate-panel and mate-control-center are all ready to use the new API changes, now need somebody to review and tests, thank you.

@lukefromdc lukefromdc requested review from raveit65 , flexiondotorg , monsta and lukefromdc and removed request for raveit65 and flexiondotorg Apr 27, 2018

@lukefromdc
Copy link
Member

lukefromdc left a comment

I just tested th this with the associated PR for mate-panel and it worked fine. Note that this PR and the panel PR must be used together in order for the panel to start. Testing m-c-c next

@lukefromdc

This comment has been minimized.

Copy link
Member

lukefromdc commented Apr 27, 2018

This change with the associated mate-panel, mate-control-center, and mozo changes worked flawlessly in the tests I just ran here. Looks ready to go on this end

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented Apr 28, 2018

@monsta @flexiondotorg
Can you please give a statement if you like this re-fork of gnome-menus or not, and should we go this way?
Before other people spend their time with reviewing ore writing more new code in other projects.

@lukefromdc lukefromdc requested a review from mate-desktop/core-team Apr 28, 2018

@monsta

This comment has been minimized.

Copy link
Member

monsta commented Apr 28, 2018

I really don't want to merge this because it's a huge commit. If something goes wrong, git bisect won't help much.

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 5, 2018

#60 is a port with less changes; use mate-desktop/mozo#41 to test Mozo with it.

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented May 6, 2018

I changed the commit history, and droped some things, now this PR only include these things:
Make MateMenuTree a GObject.
Port to pygobject-based introspection bindings.
Drop static python bindings; introspection-based ones should be used now.
Python3 support
Rewrite python example code to use introspection-based binding.
Add javascript example code to use introspection-based binding.

These things have been droped:
Add simple menu editor tool, use introspection-based bindings
Translations update
New language added.
NEWS file change
desktop-directories copies from gnome-menus
large amount of various unrelated changes across all the code of the project

@monsta How do you need to modify it now? Besides this is a huge patch.

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 6, 2018

@yetist
Does current version of this PR work with the other PRs from you now?
mozo, m-c-c, mate-screensaver and mate-panel ?

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented May 6, 2018

Yes, mozo, mate-panel, mate-control-center and mate-screensaver with this PR works fine, please see their own PRs.

@raveit65
Copy link
Member

raveit65 left a comment

Works well with latest changes and they aren't huge any more.
It's not a problem to use the few commits as patches on top of 1.20, just tested.
Also the other depending packages working all well with this changes.
I like the idea to modernize mate-menus with back porting changes from g-m, so i prefer this excellent work.
Not having a commit history for every single change from g-m is a minor issue for me and can still be ignored.

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 7, 2018

Ok, this looks better though still very large. Give me some time to adjust Debian packaging and test all this...

pyexample_DATA =
endif
exampledir = $(pkgdatadir)/examples
example_DATA = mate-menus-ls.py mate-menus-ls.js

This comment has been minimized.

@monsta

monsta May 8, 2018

Member

I don't think we need a JavaScript example. It relies on gjs, which is used by gnome-shell and most probably not installed in a typical system with MATE.

This comment has been minimized.

@yetist

yetist May 9, 2018

Author Member

Thank you for review this pr, the javascript example has been removed now.

@yetist yetist referenced this pull request May 9, 2018

Merged

Support mate-menus new api #38

@yetist

This comment has been minimized.

Copy link
Member Author

yetist commented May 9, 2018

A PR for mate-menu to use this api change is ready.

const char* desktop_entry_get_exec(DesktopEntry* entry);
void desktop_entry_add_legacy_category(DesktopEntry* src);

GLIB_DEPRECATED

This comment has been minimized.

@monsta

monsta May 11, 2018

Member

It's not a deprecation, it's a new function with old name but different return value type... It returns newly allocated string instead of a const pointer.

GLIB_DEPRECATED
char* desktop_entry_get_full_name(DesktopEntry* entry);

GLIB_DEPRECATED

This comment has been minimized.

@monsta

monsta May 11, 2018

Member

Same here

This comment has been minimized.

@monsta

monsta May 11, 2018

Member

Other "deprecations" don't have API break, but... do we really need to have our own local deprecations? We have more than enough of upstream ones.

This comment has been minimized.

@monsta

monsta May 11, 2018

Member

Even more interesting, this is a local header file. It's only used by this library, it's not installed in /usr/include for other apps to use. Then I don't understand these deprecations at all...

This comment has been minimized.

@yetist

yetist May 12, 2018

Author Member

Yes, you are right, l will remove these functions.

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 18, 2018

@monsta Is this ready to go?
...i see no activities here since 7 days.
Would be nice to test all changes from this PR and others a while before i have to push this to fedora 29 repos (rawhide).

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 19, 2018

Yeah, I've been using this and other repos with related PRs for a few days. So far looks good, though due to huge amount of changes, I could have missed some issues in the code.

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 19, 2018

@vkareh @flexiondotorg @sc0w do you guys wish to give it some more testing and/or maybe code review?

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 21, 2018

remove js example can be squashed into udpate examples commit (+fixing typo), right?
And are functions from remove useless functions commit old code from from mate-menus or new code from backport from gnome-menus ?
If the latter can this be squashed too, please?
`update version to 1.21.0' i will except here as releasing a new tarball is the next step and needed for other open PRs.

@yetist yetist force-pushed the yetist:devel branch from 2fc1a07 to c6aafdd May 21, 2018

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 21, 2018

These were internal functions (not in public API) that weren't used anymore.

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 27, 2018

Time is running out for another review, i will merge it.

@raveit65 raveit65 merged commit 047e1d2 into mate-desktop:master May 27, 2018

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 27, 2018

Thanks a lot for your hard work and patient with us ;)

@raveit65 raveit65 referenced this pull request May 27, 2018

Open

Migrate to Python 3 #27

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 27, 2018

@lukefromdc @monsta @vkareh @sc0w @flexiondotorg
I've releleased 1.21.0 but not tagged.
I think it's better to do that if all other related PRs are finished, so we can change something if needed.
A tarball is uploaded to pre-release folder at server.

@raveit65

This comment has been minimized.

Copy link
Member

raveit65 commented May 27, 2018

Last question, do we need python3 as build requires and run time?
Looks like python deps are completely dropped.

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 27, 2018

Hmm... there's no Python in configure.ac now. I think we only need GI devel packages to make our GI, same as in Atril, Caja, Pluma, etc.

@monsta

This comment has been minimized.

Copy link
Member

monsta commented May 27, 2018

For runtime deps, it depends on whether a distro packages util/mate-menus-ls.py or not. Debian/Ubuntu/Mint have it packaged, so they have runtime dependency on Python for it.

@yetist yetist deleted the yetist:devel branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment