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

Python 3 #425

Closed
wants to merge 33 commits into from

Conversation

Projects
None yet
5 participants
@monnerat
Copy link
Contributor

commented May 12, 2019

This large PR converts Pluma Python interface and its embedded plugins to Python 3.
It also fixes some Python code bugs and improves removing (although not completely) of Gtk 3 deprecated stuff in Python sources/UIs.

Thanks for considering it.

@raveit65 raveit65 requested review from monsta, sc0w and mate-desktop/core-team May 12, 2019

@yetist

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Thank you for migrating the plugin for pluma from python2 to python3.
However, this PR is too big to be reviewed.
42 files changed, 7014 insertions(+), 7113 deletions(-)

Can you split this PR into several small PRs? For example, by plugin, they are externaltools, pythonconsole, quickopen and snippets, and maybe another PR for common change.

@lukefromdc
Copy link
Member

left a comment

None of these are plugins I normally use so I really don't know how to test them but here's what I found on an attempt. Build completed fine, except for a series of translation warnings.

Snippets "add a new snippet" dialog works, but I really don't know how to use this plugin. No warnings on triggering snippets and no crash.

Python console: Could not find the python console (again, don't know how to use it, no warnings in terminal)

Selecting "Quick open" simply closes the menu

By comparison on master these can't even be enabled at all, but probably because I no longer have any python2.7 development libraries installed for space reasons.

@lukefromdc lukefromdc requested a review from mate-desktop/core-team May 14, 2019

@lukefromdc

This comment has been minimized.

Copy link
Member

commented May 14, 2019

These are the translation warnings I got:


Warning: Could not merge ca translation for msgid:
Choose <menuchoice><guisubmenu>Accessories</guisubmenu><guimenuitem>Text Editor</guimenuitem></menuchoice>.
Warning: Could not merge uk translation for msgid:
<revnumber>pluma Manual V2.6</revnumber> <date>September 2003</date> <_:revdescription-1/>
Warning: Could not merge fr translation for msgid:
Select the <guilabel>Insert spaces instead of tabs</guilabel> option to specify that <application> pluma</application> inserts spaces instead of a tab character when you press the <keycap>Tab</keycap> key.
Warning: Could not merge fr translation for msgid:
<literal>$(<replaceable>cmd</replaceable>)</literal> is replaced by the result of executing <replaceable>cmd</replaceable> in a shell.
Warning: Could not merge hu translation for msgid:
Permission is granted to copy, distribute and/or modify this document under the terms of the GNU Free Documentation License (GFDL), Version 1.1 or any later version published by the Free Software Foundation with no Invariant Sections, no Front-Cover Texts, and no Back-Cover Texts. You can find a copy of the GFDL at this <_:ulink-1/> or in the file COPYING-DOCS distributed with this manual.
@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@yetist
Thanks for your comment.
The first commit of this PR normalizes Python indenting to ts=4 and strips trailing spaces. It does not change the effective code and affects ~5800 lines. This explains why the PR is so large.

As this commit can be standalone, would it be satisfactory to you to have it alone in a first PR, then a second PR with effective code change commits (as they have to be applied together) ?

@lukefromdc
Thank you for your tests and reporting.
I do not get the translation warnings you report: maybe this is the same problem as the mate-desktop/mate-user-guide#25 discussion you participated to? In any case, this PR does not affect the help files at all and if you get these warnings with the PR applied, you should also have them without it.

The other failures you report look very similar to what I got when starting to work on this: are you sure the new Python files are used? I suspect your Pluma binary finds old files first (All 4 Python plugins work well here when installed in the F29 systems dirs).

To test the snippets (example):

  • Enable the plugin.
  • Edit a test.c file.
  • In the editing window, type if<tab>: it should complete a full if statement.
  • Type the condition expression, then <tab>: it moves the cursor to the if inner statement part.
@lukefromdc

This comment has been minimized.

Copy link
Member

commented May 14, 2019

All I get on the snippets test (with my old Pluma fully removed) is the just-typed text disappearing. As I said though I have zero understanding of its use.

@lukefromdc

This comment has been minimized.

Copy link
Member

commented May 18, 2019

As of now, in the "view" menu "Bottom Pane" is greyed out, the statusbar shows up fine, but I have no way to bring up the python console as a result of this.

@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

... with my old Pluma fully removed ...

Just to be sure: did you also remove Python cache files (*.py[co] and __pycache__ dirs) ?

@lukefromdc

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I really don't know: I removed my locally packed Debian package which does NOT install those (lots of bugs in Checkinstall) so it my be that I cannot really do a valid test and possibly my results should be ignored. I am no expert on python, so if these were ever installed by make install on a previous test of pluma with python 2 plugins I would need filenames just to find them.

I don't normally use any python plugins in Pluma but have been interested in this work because I am trying to get rid of as much of python2 as possible for space and obsolescenece reasons and this is part of getting python2 out of MATE.

@raveit65

This comment has been minimized.

Copy link
Member

commented May 20, 2019

The first commit of this PR normalizes Python indenting to ts=4 and strips trailing spaces. It does not change the effective code and affects ~5800 lines. This explains why the PR is so large.

As this commit can be standalone, would it be satisfactory to you to have it alone in a first PR, then a second PR with effective code change commits (as they have to be applied together) ?

@monnerat
That would be great. Let see how huge the PR is after you split out this commit.
Dunno if it is possible to build and test only one of the remaining plugins.

@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

As this commit can be standalone, would it be satisfactory to you to have it alone in a first PR,...

New PR for reindenting Python files: #430

... then a second PR with effective code change commits (as they have to be applied together)?

Splitting other commits would require to rework them to retain Python 2 compatibility, in order to not have things not working between applying them...

@selectiveduplicate
Copy link
Member

left a comment

We seem to have started splitting this PR commits...
Based on a first look, it builds fine...
I've had a little problem with the console plugin; I get the following warnings in the terminal when I press the preferences button for the plugin:

Traceback (most recent call last):
  File "/usr/local/lib/pluma/plugins/pythonconsole/__init__.py", line 67, in do_create_configure_widget
    return self.config_widget.configure_widget()
  File "/usr/local/lib/pluma/plugins/pythonconsole/config.py", line 98, in configure_widget
    self._ui.add_from_file(self._ui_path)
GLib.Error: gtk-builder-error-quark: /usr/local/share/pluma/plugins/pythonconsole/ui/config.ui:115:36 Invalid property: GtkFontButton.language (11)

(pluma:26305): libpeas-CRITICAL **: 23:50:49.295: show_configure_cb: assertion 'GTK_IS_WIDGET (conf_widget)' failed

and the button doesn't do anything.

The external tools plugins offer a "search" script to search in text files. When I run that command, the "search in text files" dialog doesn't pop up over my current window, I have to minimize pluma to see it; it doesn't show up on the list of windows too. After completing the search, or if I cancel the search, I get the following in my terminal:

sys:1: Warning: GChildWatchSource: Exit status of a child process was requested but ECHILD was received by waitpid(). See the documentation of g_child_watch_source_new() for possible causes.

Besides that, all the other plugins seem to work but we need further testing...

@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@selectiveduplicate
Many thanks for your report.

I've had a little problem with the console plugin; I get the following warnings in the terminal when I press the preferences button for the plugin: ...

The language property seems not supported by your gtk version. As it is not very important, I have removed it now in commit d243690. Maybe you can recheck with this.

The external tools plugins offer a "search" script to search in text files. When I run that command, the "search in text files" dialog doesn't pop up over my current window, I have to minimize pluma to see it; it doesn't show up on the list of windows too.

Yes, I noted that too and googled for it. The reason is: the popup is controlled by the zenity external program and is not under our control. The problem already occurs with the current Python 2 version (the current PR does not pretend fixing all plugins problems!). There is a dirty workaround involving the wmctrl program at https://unix.stackexchange.com/questions/152294/keep-a-zenity-dialog-box-always-on-top-in-foreground. If someone is interested in fixing it, external tools shell scripts should be updated (Run command suffers the same problem).

After completing the search, or if I cancel the search, I get the following in my terminal: ...

Strange! I do not get it and checked g_child_watch_source_new documentation as advised: it seems to me the code is conformant. In addition, nothing has been changed in the subprocess/pipe logic. Maybe you should check if this is new or if you already have it without the PR applied. I suspect this is a GLib version problem. Anyway I think you can safely ignore it.

Thanks again for your testing.

@raveit65

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I have merged #430
So this PR needs a rebase from master.

monnerat added some commits May 10, 2019

pythonconsole plugin: replace obsolete mateconf by Gio.Settings
Also use Peas.Configurable for configure invokation and adapt glade UI
using a widget instead of a dialog.
Set Python console font configable independently of Mate console.
snippets plugin: implement dual-encoding "environment"
We need to keep environment-like variables in native- and utf8-encoded byte
strings: up to now, os.environ[] was used for that purpose, but this is not
compatible with Python 3, as os.environ values are unicode strings.
This commit implements a 2-entries dictionary kept in class items for passing
our variables: entry "utf8" holds the utf-8 values and "noenc" the
native-encoded values.
These entries are itself variable dictionaries with byte string values.
@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Rebase done: PR size reduced to ~ +/-1500 lines !

@monnerat

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

I have split the remaining commits and updated the code to be Python 2 & 3 compatible. The new pull requests are:
#433
#434
#435
#436

When they will be pulled to master, a 5th one will convert Pluma itself to use Python 3.

As a consequence, I close the current PR.

@monnerat monnerat closed this May 27, 2019

@monnerat monnerat deleted the monnerat:pm-python3 branch Jun 6, 2019

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.