Desktop: Add support for Deepin desktop environment

[vdeville]

Hello,
I just push on a little modification to the Joplin Linux install script to implement desktop icon on Deepin.
Thanks

[vdeville]

@foxmask I just update following your comment.

[tessus]

@laurent22 any reason why we don't just test for ithe DESKTOP env var to be set?

Why do we have to list every possible DE out there?

[laurent22]

I have no idea, I didn't write this script so I don't know why it was done that way.

[tessus]

Ok, in that case, we should change it. Because it is somewhat annoying to change the script every time a new DE pops up or a user uses a DE that isn't in the list.

On a Linux without DE, the DESKTOP var is not set. If there are DEs for which people do not want an icon, we can still create a PR in the future. But whitelisting is the wrong approach here.

[laurent22]

What if the desktop doesn't support icons the way it's implemented here? Isn't it the reason for whiltelisting?

[vdeville]

@laurent22 Yeah i thing that is the reason.

[tessus]

That's what I thought at first, but when you look at the list, it basically lists all DEs available in current distros. (Unless I'm mistaken.)

e.g. gnome and kde couldn't work more differently, buy yet they use the same code for creating and icon? iirc the process to create an icon is differently on those 2 DEs. So maybe this is a general way to do it.

I'd change the test to [ ! -z ${DESKTOP+x} ]

[laurent22]

I'm never so keen on changing something that already works but if you're 100% sure it's not going to break something else, we can do that.

The advantage of the current script is that we built up a knowledge base with the list of desktops that we know work with the script. If we change that, I guess we might need later on to black list the desktops that don't work with it.

[laurent22]

But again I don't know enough about Linux desktops in general to have a very informed opinion about this.

[tessus]

if you're 100% sure it's not going to break something else

Nothing is 100%. You make a push of a perfectly fine change and a current fluctuation flips a bit. You'll never know what hit you. ;-) I've seen things in my job during the past 20 years you wouldn't believe.

But as far as the script goes, it won't break anything more than it does now. e.g. currently the script breaks (does nothing) when you want the icon on a DE which is not in the list.

Let's say you encounter a system that does not support the icon creation the way it is done in the script. Well, the script runs and you don't have an icon. So no change here.
Except that you have less work with updating the script.

You can't really catch all possibilities. e.g. what woud current happen, if someone on a non-DE system set DESKTOP=BLAkdeBLA manually? In this case, nothing really, but you can see where I'm going with this.

We don't have to make this change, it's just that I've noticed that every time a new DE pops up a new PR is created. IMO this is inefficient, and the change is a personal preference. That's all.
If you are ok with adding a new DE every time, by all means let's keep the current list.

[CalebJohn]

@tessus I run xubuntu (which as you know uses the xfce4 DE) and $DESKTOP is not set. I agree that what we're doing now is weird, but it does have benefits (as @laurent22 mentioned) and we need to be sure that the change we make doesn't break the current state.

[tessus]

@CalebJohn I'm a bit confused by your statement:

I run xubuntu (which as you know uses the xfce4 DE) and $DESKTOP is not set

So my proposed change doesn't change anything for you then, because it currently doesn't test true and it wouldn't test true with [ ! -z ${DESKTOP+x} ] either. So everything would be the same, or am I missing something?

[CalebJohn]

@tessus
The misunderstanding is mine, the script checks $XDG_CURRENT_DESKTOP (which is set on my system) and the stores that in a variable $desktop. I thought you were talking about a variable that is always set, not one in script. That's my bad, no objections from me.

[laurent22]

For now this PR is good anyway so let's merge, and if it makes sense we can apply the new logic in a different PR. Thanks for the update @MyTheValentinus.