Package Details: mattermost-desktop 4.1.2-2

Git Clone URL: https://aur.archlinux.org/mattermost-desktop.git (read-only)
Package Base: mattermost-desktop
Description: Mattermost Desktop application for Linux (Beta)
Upstream URL: https://github.com/mattermost/desktop
Keywords: chat mattermost messenger networking slack
Licenses: Apache
Submitter: nineinchnick
Maintainer: akstrfn (wget)
Last Packager: wget
Votes: 37
Popularity: 0.389353
First Submitted: 2016-04-02 09:08
Last Updated: 2018-07-03 16:54

Latest Comments

1 2 3 4 5 Next › Last »

ArchangeGabriel commented on 2018-07-03 17:04

Well sha256 was not in my detailed proposition, because I don’t specially advocate it. It’s just that sha512 does not bring much more, while making way longer lines.

And of course string quotation is a matter of taste (just like $var vs ${var} is when there is no issue using the former), it’s just that I prefer to see explicitly where there might be issues without quotes (which is an inconsistent reasoning with my preference for ${var}, but eh, I’m just another strange human after all). ;)

wget commented on 2018-07-03 16:56

Thanks a lot ArchangeGabriel for this kind review.

I applied your changes except two: sha and (a bit like akstrfn) string quotations. I indeed prefer to quote the whole string because my brain sees it more clearly this is a string.

I still have kept i686 and arm support for this package.

akstrfn commented on 2018-06-20 20:53

I noticed that riot also has blurry icon but anyway I tried googleing around and reading electron docs to figure it out. I had an impression that it can be solved with js launcher instead of bash launcher. Good that you told me that it is a known bug ;)

ArchangeGabriel commented on 2018-06-20 20:39

I guessed something like that for the *, but since we removed tools to build as i686, I did not investigate further.

The tray icon is not a Mattermost issue, all electron apps have blurry icons in KDE systray (riot.im, cozy-desktop, slack-desktop…). It’s an electron/KDE issue, nothing you can fix in the PKGBUILD.

akstrfn commented on 2018-06-20 20:36

Thanks for the tips @ArchangeGabriel. * in cp line is there for the i686 build since you have something like linux-i32-unpacked or something similar in that arch.

I like to quote the whole path, instead of just e.g. {pkgdir} since it looks nicer to me, but its a matter of taste :)

There is one problem with the current build that I didn't explore further. The tray icon in my KDE system is blurry. I didn't figure out how to fix that when launching with shared electron binary.

ArchangeGabriel commented on 2018-06-20 20:00

I’m finally letting you add this package to your application.

Here are my suggested changes:

source=(${pkgname}-${pkgver}.tar.gz::"${url}/archive/v${pkgver}.tar.gz"
        "${pkgname}.sh"
        "${pkgname/-/.}")

Explanations: you must have an unique name for the source tarball in order to avoid conflict with other packages, hence the need to rename it. As a compensation somehow, you can use ${url} to simplify the line. The last line requires you to rename Mattermost.desktop to mattermost.desktop, which is more standard anyway. Note that there could be a solution (using ^ if I remember well) to even keep the first uppercase letter, but I don’t think it’s a good idea as stated above.

Then, you can change the three instances of cd "${srcdir}/desktop-${pkgver}" by cd desktop-${pkgver} since all functions start into ${srcdir}.

The npm run package:linux --cache "${srcdir}/npm-cache" action should rather be inside package(), because it’s a packaging action rather than a building one.

You don’t need to specify -m 755 for folders, since this is the default. I’m not sure why you have an * in the cp line, but I would rather write this: cp -r release/linux-unpacked/resources "${pkgdir}"/usr/lib/${pkgname} Oh and yes, btw, you only need to quote ${srcdir} and ${pkgdir}, there is no need to quote the rest of those paths.

I would move this line:

install -Dm644 LICENSE.txt "$pkgdir/usr/share/licenses/$pkgname/LICENSE"

before the bin part (see below why) and rewrite it like this:

install -Dm644 LICENSE.txt -t "${pkgdir}"/usr/share/licenses/${pkgname}

It’s OK for the licenses file to be called anything like COPYRIGHT, LICENSE, same things with .txt or .md extensions, etc. There are no rules regarding this, so using upstream one is the easiest.

Then I would also move this line up:

install -Dm644 "$srcdir/desktop-$pkgver/release/linux-unpacked/icon.png" "$pkgdir/usr/share/pixmaps/$pkgname.png"

and rewrite it like this:

install -Dm644 resources/linux/icon.png "${pkgdir}"/usr/share/icons/hicolor/512x512/apps/${pkgname}.png

Using pixmaps is deprecated AFAIK, and this is way better. Do note that the next version is bringing an SVG icon instead, so this line should then become:

install -Dm644 resources/linux/icon.svg "${pkgdir}"/usr/share/icons/hicolor/scalable/apps/${pkgname}.svg

Then we have the bin and .desktop parts remaining. At this point, to simplify writing and more explicitly separate what is provided locally and what is from upstream, I would do a cd "${srcdir}", and then:

install -Dm755 ${pkgname}.sh "${pkgdir}"/usr/bin/${pkgname}
install -Dm644 ${pkgname/-/.} -t "${pkgdir}"/usr/share/applications/

Note that you don’t have to create the bin dir beforehand, the -D option of install takes care of it. Oh and if you prefer, you could even do this at the beginning of the package() function to avoid the cd "${srcdir}" line completely.

Oh, and you can remove your npm “hack”, it has been fixed in the 6.1.0 release, which is Arch Linux current one.

For reference, here is the PKGBUILD I intended for [community]. Note that it removes i686 support, so they are some additional simplifications that do not apply as long as the package stays in the AUR.

Finally, you should change the mattermost.desktop (renamed from Mattermost.desktop, remember) file like this:

[Desktop Entry]
Name=Mattermost
Comment=Mattermost Desktop application for Linux
Exec=/usr/bin/mattermost-desktop
Terminal=false
Type=Application
Icon=mattermost-desktop
Categories=Network;InstantMessaging;

The Exec line is best practice FWICS, the icon one allows finding the most suited one as well as theming by using other icons sets rather than hardcoding the pixmap version, and the last line is to avoid the icon to end in “Lost&Found” in desktop menus (or whatever the exact name is in english) and was also added upstream for next version.

ArchangeGabriel commented on 2018-06-15 07:31

Oh and finally, you will be very welcome to co-maintain this in [community]. This does also counts towards being accepted. ;)

ArchangeGabriel commented on 2018-06-15 07:27

Well, I planned to move this for the same reason I used to plan to move mattermost itself some time ago: because I use it (which is enough by itself) and also because I’ve made improvements to the PKGBUILD.

You’re definitively doing a better job than I would regarding mattermost itself, and honestly that one + openct and some of your fonts related packages are more than enough to become a TU (some TUs really started with one package only). ;)

wget commented on 2018-06-13 15:22

@ArchangeGabriel actually I was planning to request an application to become an Arch Linux Trusted User (within 3 months) and wanted to move the whole Mattermost software stack (+third party stable extensions) to community. This would have been an argument in favour of my application. :-/ What would you recommend? Why are you planning to move this to community any reason?

akstrfn commented on 2018-06-07 17:08

@ArchangeGabriel I am fine with it :)