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

Added an extension for opening a symlink's parent directory #32

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added an extension for opening a symlink's parent directory #32

wants to merge 6 commits into from

Conversation

CreamyCookie
Copy link

@lukefromdc lukefromdc requested a review from a team June 30, 2018 22:25
@raveit65
Copy link
Member

Anyone around with python experience to review this PR?

@vkareh
Copy link
Member

vkareh commented Jul 16, 2018

@raveit65 - I tried testing this but wasn't able to get any of the example plugins to load... I'll give it another try later this week, but it might be something in my caja build configuration that I'll have to check.

@CreamyCookie
Copy link
Author

CreamyCookie commented Jul 17, 2018

Okay, I just realized that there's no reason to not make it work with directory symlinks, so it now does.

@CreamyCookie
Copy link
Author

CreamyCookie commented Jul 18, 2018

Sorry for all the additional commits. I think it's finished now.

Should I create new pull request that only contains the main commit (but with the fixes etc. merged)?

@vkareh
Copy link
Member

vkareh commented Jul 18, 2018

@CreamyCookie - you can just squash your commits instead into a single one (or we can do it during merge - github has a button for squashing on my view)

@raveit65
Copy link
Member

@vkareh
If you choose squash on github site than you generate a merge commit.
Better do that local with git rebase -i last-good-commit-id
i= interactive

@CreamyCookie
Copy link
Author

Interesting, didn't know GitHub had that feature.

@raveit65 Maybe I'm misunderstanding, but the docs seem to suggest that it does get squashed to a single commit, i.e. no additional merge commit:

https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits

@raveit65
Copy link
Member

Try it out in a own test repo, it produce garbage.

@CreamyCookie
Copy link
Author

CreamyCookie commented Jul 18, 2018

@raveit65 Huh? Worked perfectly fine on my end: https://github.com/CreamyCookie/test (squashed and merged the feature branch which contains the 3 new commits)

@vkareh
Copy link
Member

vkareh commented Jul 18, 2018

@raveit65 - do you know how to build this so that the example plugins load in caja? They just don't show up for me...

@CreamyCookie
Copy link
Author

@vkareh For me it works, just putting them in ~/.local/share/caja-python/extensions and then killing and restarting caja.

@raveit65
Copy link
Member

@vkareh
I don't know , i used the package only for caja-terminal (embeded terminal) for a while.
But since i dropped this package for fedora i don't use caja-python any more.
Currently i am thinking about to drop python-caja for fedora too, to reduce my workload.

@monsta
Copy link
Contributor

monsta commented Jul 19, 2018

"Squash and merge" button here should not generate an additional merge commit... it will only place PR number into first line of commit message - but you can delete that part manually before merging.

Besides the mentioned local folder, Python extensions can be placed into /usr/share/caja-python/extensions/.

Copy link
Contributor

@monsta monsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, just need to add this new file to examples/Makefile.am, otherwise it's not installed

@raveit65
Copy link
Member

Is this python-3.6 compatible?

@CreamyCookie
Copy link
Author

@raveit65 If you mean this extension, then yes AFAICS.

@raveit65
Copy link
Member

python-caja can easily build with python-3.6, see this PR for fedora downstream package
https://src.fedoraproject.org/rpms/python-caja/pull-request/2
It would be hurt to break this.

@CreamyCookie
Copy link
Author

CreamyCookie commented Jul 19, 2018

When I was looking at the code regarding Python 3.6 compatibility (to be sure), I've noticed another potential bug. This line wouldn't work with paths that contain double quotes:

os.system('caja "%s" &' % parent)

So, it's now using the subprocess module which is recommended anyway and also handles escaping. Oh, and instead of calling caja for each path, I now call it once with all the paths.

@monsta
Copy link
Contributor

monsta commented Aug 5, 2018

Hmm, I didn't notice it before, but there's a runtime warning in ~/.xsession-errors:

RuntimeError: object at 0x7fa52623b140 of type OpenSymLinksParentDirsExtension is not initialized

@CreamyCookie
Copy link
Author

CreamyCookie commented Aug 5, 2018

Interesting, I'm also getting that for the plugins included in Ubuntu MATE:


RuntimeError: object at 0x7f722686d8c0 of type OpenSymLinksParentDirsExtension is not initialized
RuntimeError: object at 0x7f722686d5f0 of type FolderColorMenu is not initialized
RuntimeError: object at 0x7f722686d5a0 of type RenameMenu is not initialized

Hmm.. could this be a result of my empty __init__?

Oh completely missed this comment of yours:

It works, just need to add this new file to examples/Makefile.am, otherwise it's not installed

Should I add my file to examples/Makefile.am ? Anything else that is blocking this? (Besides maybe the above xsession-errors)

@raveit65
Copy link
Member

Any news on this.
@mate-desktop/core-team
Can someone answered last questions from author, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants