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

Create bcompare.profile #4098

Merged
merged 17 commits into from
Mar 19, 2021
Merged

Create bcompare.profile #4098

merged 17 commits into from
Mar 19, 2021

Conversation

tredondo
Copy link
Contributor

@tredondo tredondo commented Mar 14, 2021

My first profile, please review and squash the commits 😁

In particular, I don't really understand what ipc-namespace is for, but the template says it's for CLI-only apps, so I've left it commented out.

Also unclear on the private-* and dbus stuff.

Copy link
Collaborator

@rusty-snake rusty-snake left a comment

Choose a reason for hiding this comment

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

Looks good, some nitpicks to go.

  • add it to src/firecfg/firecfg.config
  • if it has it's own configuration files/dirs, add them to disable-programs.inc

etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
Comment on lines +21 to +22
# Uncommenting this breaks launch
# include disable-shell.inc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Uncommenting this breaks launch
# include disable-shell.inc

no need to keep such lines (in general it's often enough to include allow-bin-sh.inc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems a bit arbitrary... disable all shells, but then enable back bash, dash, and sh, but not the much more popular zsh. Why bother disabling shells then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

zsh is popular as interactive shell yes, but not to write script in it as you can not assume it's installed on system. Usually you write you scripts for /bin/sh which should be present on any POSIX compatible system or /bin/bash which should be present on the most GNU/Linux systems. allow-bin-sh.inc does exactly what it names says, it allows /bin/sh. /bin/sh is a symlink to /bin/bash on the most system resp. /bin/dash on Debian/Ubuntu. Therefore they need a noblacklist too.

Comment on lines +24 to +25
# Don't disable ${DOCUMENTS}, ${MUSIC}, ${PICTURES}, ${VIDEOS}
# include disable-xdg.inc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Don't disable ${DOCUMENTS}, ${MUSIC}, ${PICTURES}, ${VIDEOS}
# include disable-xdg.inc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've kept such lines because to me it was surprising to realize that xdg was related to ${DOCUMENTS}, and I thought others who base the profile on this one might benefit from the comment. For the same reason, I added potentially superfluous comments like

# Might break Pulse Audio
#machine-id

or

# Allow associated applications to play sound files
#nosound

etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
etc/profile-a-l/bcompare.profile Outdated Show resolved Hide resolved
# In case the user decides to include disable-programs.inc, still allow KDE's Gwenview to view images via right click -> Open With -> Associated Application
noblacklist ${HOME}/.config/gwenviewrc

include disable-common.inc
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest that we use the same comment format as in meld.profile and kdiff3.profile: Uncomment the next line (or put it into your meld.local) if you don't need to compare files in FOO

Suggested change
include disable-common.inc
# Uncomment the next line (or put it into your meld.local) if you don't need to compare files in disable-common.inc
#include disable-common.inc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed with meld replaced by bcompare.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant to use this message for the others (disable-programs.inc, wusc, ..) too.

@tredondo
Copy link
Contributor Author

* add it to src/firecfg/firecfg.config
* if it has it's own configuration files/dirs, add them to disable-programs.inc

Added those instructions to the wiki.

@tredondo
Copy link
Contributor Author

Should I add noblacklist ${HOME}/.config/bcompare, given that I've added bcompare to disable-programs.inc?

@tredondo
Copy link
Contributor Author

@rusty-snake / others, can we have a look-over and see what really needs to be done to merge this PR?

I'm somewhat partial to the comments I left above disable-xdg and disable-shell, but if I must, I can remove them.

@netblue30 netblue30 merged commit a7acaa6 into netblue30:master Mar 19, 2021
@netblue30
Copy link
Owner

Merged, thanks!

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.

None yet

3 participants