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

Fix for duplicate Python extensions in management interface #521

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@alexandervdm
Member

alexandervdm commented Mar 7, 2016

This commit fixes issue #520. Note that python-caja pull-request 24 is also required for this solution.

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 8, 2016

Member

Thanks - but it doesn't build here 😞

  CC       caja-module.lo
caja-module.c: In function ‘add_module_objects’:
caja-module.c:173:31: error: ‘pyfiles’ undeclared (first use in this function)
         module->list_pyfiles(&pyfiles);
                               ^
caja-module.c:173:31: note: each undeclared identifier is reported only once for each function it appears in

Also I thought that it would be nice to have some protection against loading older version of libpython-caja.so. When list_pyfiles function isn't found in the module, there should be no attempt to call it...

Member

monsta commented Mar 8, 2016

Thanks - but it doesn't build here 😞

  CC       caja-module.lo
caja-module.c: In function ‘add_module_objects’:
caja-module.c:173:31: error: ‘pyfiles’ undeclared (first use in this function)
         module->list_pyfiles(&pyfiles);
                               ^
caja-module.c:173:31: note: each undeclared identifier is reported only once for each function it appears in

Also I thought that it would be nice to have some protection against loading older version of libpython-caja.so. When list_pyfiles function isn't found in the module, there should be no attempt to call it...

Update caja-module.c
Fix missing GList init.
@alexandervdm

This comment has been minimized.

Show comment
Hide comment
@alexandervdm

alexandervdm Mar 8, 2016

Member

Fixed the missing variable initialisation. Sorry about that. I manually merged my 1.10 based work against master.. so I was asking for it. I agree with your other comment as well, will have to look into it once I get off work. Thanks for the feedback!

Member

alexandervdm commented Mar 8, 2016

Fixed the missing variable initialisation. Sorry about that. I manually merged my 1.10 based work against master.. so I was asking for it. I agree with your other comment as well, will have to look into it once I get off work. Thanks for the feedback!

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 8, 2016

Member

Builds fine now, thanks. I've tested it with git master version of python-caja, and as I suspected, Caja crashes right on startup (when python extension is enabled), then session manager restarts it, then it crashes again, etc. So the protection is indeed required here. I'll wait for your fix 😄

Regarding the first commit: as I understand it, it doesn't require changes in python-caja?
I'm asking because I'd like to put 8a826a6 and the first commit from here into 1.12 branch and make a new 1.12.x release. The fixes are small and they should be enough for 1.12.x users.
Other commits from here are bigger and they also add a new feature, so they should go into 1.14 only. 😄

Member

monsta commented Mar 8, 2016

Builds fine now, thanks. I've tested it with git master version of python-caja, and as I suspected, Caja crashes right on startup (when python extension is enabled), then session manager restarts it, then it crashes again, etc. So the protection is indeed required here. I'll wait for your fix 😄

Regarding the first commit: as I understand it, it doesn't require changes in python-caja?
I'm asking because I'd like to put 8a826a6 and the first commit from here into 1.12 branch and make a new 1.12.x release. The fixes are small and they should be enough for 1.12.x users.
Other commits from here are bigger and they also add a new feature, so they should go into 1.14 only. 😄

@alexandervdm

This comment has been minimized.

Show comment
Hide comment
@alexandervdm

alexandervdm Mar 8, 2016

Member

Thanks for testing - I'll get the protection code to you as soon as I can. :)

You are correct in that 897eec2 and 8a826a6 together fix this issue and the one described in #22 and do not require changes to python-caja.
The only downside of including only these two is that python extensions cannot be managed individually, but a bugfix release that includes these commits would still improve the python extension situation a lot.

Member

alexandervdm commented Mar 8, 2016

Thanks for testing - I'll get the protection code to you as soon as I can. :)

You are correct in that 897eec2 and 8a826a6 together fix this issue and the one described in #22 and do not require changes to python-caja.
The only downside of including only these two is that python extensions cannot be managed individually, but a bugfix release that includes these commits would still improve the python extension situation a lot.

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 11, 2016

Member

BTW, if you don't mind, I can merge 897eec2 already, and then you can rebase your branch (and probably rename this PR 😄).

Member

monsta commented Mar 11, 2016

BTW, if you don't mind, I can merge 897eec2 already, and then you can rebase your branch (and probably rename this PR 😄).

@flexiondotorg

This comment has been minimized.

Show comment
Hide comment
@flexiondotorg

flexiondotorg Mar 12, 2016

Member

@alexandervdm Thanks for working on this ☺️ Do you have an ETA for the protection code? I need a fix for this quite urgently and my offer to reward your efforts still stands.

Member

flexiondotorg commented Mar 12, 2016

@alexandervdm Thanks for working on this ☺️ Do you have an ETA for the protection code? I need a fix for this quite urgently and my offer to reward your efforts still stands.

@alexandervdm

This comment has been minimized.

Show comment
Hide comment
@alexandervdm

alexandervdm Mar 13, 2016

Member

d0bd3f4 adds protection code around the module loading. Tested OK on original and the new python-caja. If there's any issues, please let me know.

Python extension don't include the .caja-extension files that expose details (name, description, author, etc) for C extensions shown in the interface above. I'll look into a Python variation for this mechanism. It will require a general change to caja and changes to individual python extensions to support this. Is this a patch you'd accept still for 16.04, or is this a next release kind of patch either way?

Member

alexandervdm commented Mar 13, 2016

d0bd3f4 adds protection code around the module loading. Tested OK on original and the new python-caja. If there's any issues, please let me know.

Python extension don't include the .caja-extension files that expose details (name, description, author, etc) for C extensions shown in the interface above. I'll look into a Python variation for this mechanism. It will require a general change to caja and changes to individual python extensions to support this. Is this a patch you'd accept still for 16.04, or is this a next release kind of patch either way?

@alexandervdm

This comment has been minimized.

Show comment
Hide comment
@alexandervdm

alexandervdm Mar 13, 2016

Member

451cc95 is an optional addition that sets a default name/description for python modules. Example.

Member

alexandervdm commented Mar 13, 2016

451cc95 is an optional addition that sets a default name/description for python modules. Example.

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 13, 2016

Member

Works good, thanks! 😄

Member

monsta commented Mar 13, 2016

Works good, thanks! 😄

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 13, 2016

Member

Python extension don't include the .caja-extension files that expose details (name, description, author, etc) for C extensions shown in the interface above. I'll look into a Python variation for this mechanism. It will require a general change to caja and changes to individual python extensions to support this. Is this a patch you'd accept still for 16.04, or is this a next release kind of patch either way?

My opinion is that this new feature can go into next release.
I'm not sure if it should be done via .caja-extension files: we might use some special names in the comments at the start of .py files, for example.

Member

monsta commented Mar 13, 2016

Python extension don't include the .caja-extension files that expose details (name, description, author, etc) for C extensions shown in the interface above. I'll look into a Python variation for this mechanism. It will require a general change to caja and changes to individual python extensions to support this. Is this a patch you'd accept still for 16.04, or is this a next release kind of patch either way?

My opinion is that this new feature can go into next release.
I'm not sure if it should be done via .caja-extension files: we might use some special names in the comments at the start of .py files, for example.

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 13, 2016

Member

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have python-caja installed and have some Python extensions (more than one!) in /usr/share/caja-python/extensions/ or ~/.local/share/caja-python/extensions/. You can get a few extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list (#520 should be fixed)

What should not happen:

  • any crashes or new warnings 😄

What should happen if you also apply mate-desktop/python-caja#24 to python-caja:

  • every Python extension is listed as a separate entry in the extensions list
Member

monsta commented Mar 13, 2016

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have python-caja installed and have some Python extensions (more than one!) in /usr/share/caja-python/extensions/ or ~/.local/share/caja-python/extensions/. You can get a few extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list (#520 should be fixed)

What should not happen:

  • any crashes or new warnings 😄

What should happen if you also apply mate-desktop/python-caja#24 to python-caja:

  • every Python extension is listed as a separate entry in the extensions list
@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 13, 2016

Member

@alexandervdm: I see a bit of inconsistency here though... when mate-desktop/python-caja#24 is applied and some Python extensions are installed, there's no separate entry for python-caja itself in the list, so you basically can't disable it via UI.

What do you think about it guys?

Member

monsta commented Mar 13, 2016

@alexandervdm: I see a bit of inconsistency here though... when mate-desktop/python-caja#24 is applied and some Python extensions are installed, there's no separate entry for python-caja itself in the list, so you basically can't disable it via UI.

What do you think about it guys?

@lukefromdc

This comment has been minimized.

Show comment
Hide comment
@lukefromdc

lukefromdc Mar 13, 2016

Member

I've never had any python extensions installed for Caja and have had plenty of
issues with python in the past with BS like checkinstall wanting to put
everything in the wrong place.

Maybe tomorrow I can play with this, but I've yet to even build python-caja and
will have to get it working first, probably with the example extensions mentioned,
Have a major video editing projectnto finish first.

On 3/13/2016 at 1:08 PM, "monsta" notifications@github.com wrote:

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have
python-caja installed and have some Python extensions (more than
one!) in /usr/share/caja-python/extensions/ or
~/.local/share/caja-python/extensions/. You can get a few
extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list
    (#520 should be fixed)

What should not happen:

  • any crashes or warnings 😄

What should happen if you also apply https://github.com/mate-
desktop/python-caja/pull/24 to python-caja:

  • every Python extension is listed as a separate entry in the
    extensions list

Reply to this email directly or view it on GitHub:
#521 (comment)
196002361

Member

lukefromdc commented Mar 13, 2016

I've never had any python extensions installed for Caja and have had plenty of
issues with python in the past with BS like checkinstall wanting to put
everything in the wrong place.

Maybe tomorrow I can play with this, but I've yet to even build python-caja and
will have to get it working first, probably with the example extensions mentioned,
Have a major video editing projectnto finish first.

On 3/13/2016 at 1:08 PM, "monsta" notifications@github.com wrote:

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have
python-caja installed and have some Python extensions (more than
one!) in /usr/share/caja-python/extensions/ or
~/.local/share/caja-python/extensions/. You can get a few
extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list
    (#520 should be fixed)

What should not happen:

  • any crashes or warnings 😄

What should happen if you also apply https://github.com/mate-
desktop/python-caja/pull/24 to python-caja:

  • every Python extension is listed as a separate entry in the
    extensions list

Reply to this email directly or view it on GitHub:
#521 (comment)
196002361

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 14, 2016

Member

@alexandervdm: dammit... 897eec2, applied without later commits, actually prevents 8a826a6 from working 😞
Check 1.12 branch. Looks like I've made the release too soon...

Member

monsta commented Mar 14, 2016

@alexandervdm: dammit... 897eec2, applied without later commits, actually prevents 8a826a6 from working 😞
Check 1.12 branch. Looks like I've made the release too soon...

@lukefromdc

This comment has been minimized.

Show comment
Hide comment
@lukefromdc

lukefromdc Mar 15, 2016

Member

My test was not sucessful, in a build of python-caja and both these
changes everything built and installed, but not one of the example
extensions showed up at all after copying them over to
/usr/share/caja-python/extensions/ . Going into the preferences
dialog/extensions after restarting Caja showed only the extensions
that were already there, not the new Python ones.

Could this be another GTK 3 issue? I got a configuration warning for
python-caja about --with-gtk being not a rcognized option, rather
like with Mozo. Mozo never worked with GTK3 until the code was
changed to support it, now it is GTK3 only and works just fine.

On 3/13/2016 at 1:08 PM, "monsta" notifications@github.com wrote:

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have
python-caja installed and have some Python extensions (more than
one!) in /usr/share/caja-python/extensions/ or
~/.local/share/caja-python/extensions/. You can get a few
extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list
    (#520 should be fixed)

What should not happen:

  • any crashes or warnings 😄

What should happen if you also apply https://github.com/mate-
desktop/python-caja/pull/24 to python-caja:

  • every Python extension is listed as a separate entry in the
    extensions list

Reply to this email directly or view it on GitHub:
#521 (comment)
196002361

Member

lukefromdc commented Mar 15, 2016

My test was not sucessful, in a build of python-caja and both these
changes everything built and installed, but not one of the example
extensions showed up at all after copying them over to
/usr/share/caja-python/extensions/ . Going into the preferences
dialog/extensions after restarting Caja showed only the extensions
that were already there, not the new Python ones.

Could this be another GTK 3 issue? I got a configuration warning for
python-caja about --with-gtk being not a rcognized option, rather
like with Mozo. Mozo never worked with GTK3 until the code was
changed to support it, now it is GTK3 only and works just fine.

On 3/13/2016 at 1:08 PM, "monsta" notifications@github.com wrote:

@raveit65 @XRevan86 @lukefromdc
Please test it if you can guys. For the test you'll need to have
python-caja installed and have some Python extensions (more than
one!) in /usr/share/caja-python/extensions/ or
~/.local/share/caja-python/extensions/. You can get a few
extensions from examples dir of python-caja source tree.

What should happen:

  • no duplicate "Python" entries in the extensions list
    (#520 should be fixed)

What should not happen:

  • any crashes or warnings 😄

What should happen if you also apply https://github.com/mate-
desktop/python-caja/pull/24 to python-caja:

  • every Python extension is listed as a separate entry in the
    extensions list

Reply to this email directly or view it on GitHub:
#521 (comment)
196002361

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 22, 2016

Member

Ok, looks like I've missed an important thing during my earlier testing... indeed, this PR alone (without mate-desktop/python-caja#24) eliminates the duplicates from the extensions list, but multiple Python extensions don't work either.

I think 897eec2 prevents them from working once again. 😕

Member

monsta commented Mar 22, 2016

Ok, looks like I've missed an important thing during my earlier testing... indeed, this PR alone (without mate-desktop/python-caja#24) eliminates the duplicates from the extensions list, but multiple Python extensions don't work either.

I think 897eec2 prevents them from working once again. 😕

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 22, 2016

Member

@lukefromdc: well, python-caja works fine for me in GTK+3 build. I actually see one Python extension loaded and showing a right-click menu item (it's folder-color extension).

Member

monsta commented Mar 22, 2016

@lukefromdc: well, python-caja works fine for me in GTK+3 build. I actually see one Python extension loaded and showing a right-click menu item (it's folder-color extension).

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 23, 2016

Member

@alexandervdm: so, here are more test results:

  • this PR alone:
    • as is: no duplicates, multiple extensions don't work;
    • with 897eec2 reverted: duplicates are present, multiple extensions work;
  • this PR + mate-desktop/python-caja#24:
    • as is: no duplicates, multiple extensions work. Also every Python extension is listed, but python-caja itself isn't (see #521 (comment));
    • with 897eec2 reverted: same. O_o

With that said, I think 897eec2 isn't needed and can be removed.

Member

monsta commented Mar 23, 2016

@alexandervdm: so, here are more test results:

  • this PR alone:
    • as is: no duplicates, multiple extensions don't work;
    • with 897eec2 reverted: duplicates are present, multiple extensions work;
  • this PR + mate-desktop/python-caja#24:
    • as is: no duplicates, multiple extensions work. Also every Python extension is listed, but python-caja itself isn't (see #521 (comment));
    • with 897eec2 reverted: same. O_o

With that said, I think 897eec2 isn't needed and can be removed.

@monsta

This comment has been minimized.

Show comment
Hide comment
@monsta

monsta Mar 29, 2016

Member

Merged, except for 897eec2:

Member

monsta commented Mar 29, 2016

Merged, except for 897eec2:

@monsta monsta closed this Mar 29, 2016

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