deepin.deepin-screenshot: init at 4.1.8 #59242
Conversation
|
Notice that |
👍 Looked like it was optional functionality |
There was a problem hiding this comment.
There's /usr/bin/deepin-screenshot in the dbus service and I think we should hardcode
everything in the .desktop file because it wants to use deepin-turbo-invoker.
Exec=deepin-turbo-invoker --type=dtkwidget deepin-screenshot --icon
Note: the desktop file has multiple actions
|
Also appears to fallback to I'm thinking about patching away it trying to use |
|
@GrahamcOfBorg build deepin-screenshot |
worldofpeace
changed the title
|
@GrahamcOfBorg build deepin.deepin-screenshot |
| ]; | ||
|
|
||
| buildInputs = [ | ||
| # dde-file-manager |
There was a problem hiding this comment.
did you already figure out whether dde-file-manager is needed or not?
Maybe add a small comment here…
There was a problem hiding this comment.
Right…
It might make sense it opens in a deepin desktop environment to open with dde-file-manager, but not in general. A hardcoded /usr/bin/dde-file-manager is ugly, for sure ;-)
I'm not sure how this is handled for example in gnome-shell, to make sure folders are opened with nautilus. Is that also just an xdg-open thing?
If that's the case, I'd be inclined to solely rely on xdg-open for deepin-screenshot, and even send the patch upstream (given it should work with xdg-open for them, too)
There was a problem hiding this comment.
I'm not sure how this is handled for example in gnome-shell, to make sure folders are opened with nautilus. Is that also just an xdg-open thing?
It's just mime type associations. So the default file manager will be used, and in a deepin environment it would be dde-file-manager.
If that's the case, I'd be inclined to solely rely on xdg-open for deepin-screenshot, and even send the patch upstream (given it should work with xdg-open for them, too)
I'd say that's reasonable, though there's still room for improvement.
I'll put the patch here for now, and pr it to deepin 👍
There was a problem hiding this comment.
- patch here f3c5992
- deepin pr Only use xdg-open to launch file-manager martyr-deepin/deepin-screenshot#52
There was a problem hiding this comment.
From the commit message:
deepin.deepin-screenshot: only use xdg-open
This doesn't hardcode or put it into PATH
as I wasn't sure what romildo would have preferred.
To have xdg-open in PATH is it enough to add it to the buildInputs list, or some sort of wrapping is also needed? If not needed, that is the easier choice, in my opinion.
There was a problem hiding this comment.
From the commit message:
deepin.deepin-screenshot: only use xdg-open
This doesn't hardcode or put it into PATH
as I wasn't sure what romildo would have preferred.
To have xdg-open in PATH is it enough to add it to the buildInputs list, or some sort of wrapping is also needed? If not needed, that is the easier choice, in my opinion.
There was a problem hiding this comment.
Usually hardcoding, like you've been doing, is preferred.
@worldofpeace then it is my choice too. Can you do that, please?
|
Pushed another version that uses I looked into other places in nixpkgs on how it's done, it seems for GTK applications where For QT application, we currently don't have a wrapper (but might get one soon: #54525), so I think hardcoding this for now is fine. @worldofpeace good to merge? |
Co-authored-by: worldofpeace <worldofpeace@users.noreply.github.com> Co-authored-by: Florian Klink <flokli@flokli.de>
|
Just had to add Looks good, merging 👍
We just suffixes |
I would keep them because they help a lot spotting FHS hard coded paths in the source. There is the possibility that some of them may have unseen, or not handled yet, or introduced in new versions. |
That should be: Fixed in #59429 |
Motivation for this change
Add deepin-screenshot, a easy-to-use screenshot tool for Deepin Desktop Environment.
About packaging deepin for NixOS: #59023
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)