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

Linux Love and Care: Major rewrite which adds support for Linux Native, Flatpaks, Snaps and refactors most of the code #282

Open
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

Arcitec
Copy link

@Arcitec Arcitec commented Aug 23, 2022

Hello Everyone! 🐈

Here's a little "love and care" package for Linux users.

The commit descriptions explain all of the changes in detail.

You can also thank @TayouVR for asking me nicely if I could update the installer. I had a little bit of time left over and thought "ehh, why not?". It wouldn't have happened if they hadn't asked. But BetterDiscord is a great project, so I'm happy to help! 😜

 

Works Action Version
Install Linux Native: Discord
Install Linux Native: Canary
Install Linux Flatpak: Discord
Install Linux Flatpak: Canary
Install Linux Snap: Discord
Install Linux Snap: Canary
Repair Linux Native: Discord
Repair Linux Native: Canary
Repair Linux Flatpak: Discord
Repair Linux Flatpak: Canary
Repair Linux Snap: Discord
Repair Linux Snap: Canary
Uninstall Linux Native: Discord
Uninstall Linux Native: Canary
Uninstall Linux Flatpak: Discord
Uninstall Linux Flatpak: Canary
Uninstall Linux Snap: Discord
Uninstall Linux Snap: Canary
Browse Linux Native: Discord
Browse Linux Native: Canary
Browse Linux Flatpak: Discord
Browse Linux Flatpak: Canary
Browse Linux Snap: Discord
Browse Linux Snap: Canary
Kill/Restart Linux Native: Discord
Kill/Restart Linux Native: Canary
Kill/Restart Linux Flatpak: Discord
Kill/Restart Linux Flatpak: Canary
Kill/Restart Linux Snap: Discord
Kill/Restart Linux Snap: Canary

(The "Browse" action means clicking on the installer's Browse button to manually choose the installation folder.)

 

Downloadable version while you wait:

Due to the extensive code rewrites, this pull request may take a long time to review.

People who are eager to get started can download the v1.2.0 Linux AppImage:

https://github.com/Bananaman/BetterDiscord-Installer/releases/tag/v1.2.0

That AppImage will be removed when it's officially merged.

 

Issues fixed by this pull request:

(Sorry if I forgot to add any more related tickets. 🤣)

We need Electron 13 so that we can successfully launch on modern Linux distros which have a new version of "glibc".

However, we can't go beyond Electron 13 since the "remote" API which this application is entirely built around has been removed in newer versions. They now require that all applications use sockets to talk to the backend to interact with the operating system, rather than directly interacting via calls to "remote" in the GUI code. So a switch to Electron 14 or newer would require a heavy rewrite of this project.

All other dependencies have also been updated to fix bugs in the dependencies. And the "tmp" dependency has been added for generation of safe, cross-platform temp filenames.
- The per-platform path discovery algorithms have now been separated into individual functions for easier maintainability.

- The code now supports finding multiple paths, if the person has installed the client in multiple locations (such as Flatpak, Snap and Native). However, since all the other code expects a single path, we currently just grab the first path we find, prioritizing Flatpak paths over Snap paths over Native paths, in that order.

- The Linux code for finding the Discord path now uses the proper "xdg-config" variable (~/.config).

- Linux support has been added for Flatpaks, Snaps and Native installations of Discord. The hardcoded config paths we use are completely universal for all machines, since they are standardized by Flatpak and Snap, respectively. For native, we respect xdg-config as intended.

- Version detection has been refactored into a single function that is called everywhere in the code, rather than having 5 spread out code locations that all contain their own implementations of the same thing (which was a potential source of future bugs).

- Added comments to all code to help future maintainers/contributors.
- Raised relaunch delay from 1s to 1.5s for all platforms. It's safer for people with slow computers.

- Implemented detection of whether a Linux binary was launched as a Flatpak, Snap or Native.

- Added support for launching Discord's Linux Flatpaks, Snaps and Native, via their proper launch methods ("flatpak run", "snap run" or directly calling the native binary), along with now using the correct way to "spawn()" binaries on Linux (the old code used shell.openPath which does not work on Linux at all). All launch methods now set the correct "working directory" for the launched binary, to ensure that everything launches properly.

- Improved the GUI's logging messages. Words like "kill" have been replaced with "close", since non-technical users are the primary audience of BetterDiscord-Installer.

- Added detailed comments for future maintainers and contributors.
- Functions now take arguments rather than abusing global variables.

- Fixed the BetterDiscord ASAR downloading code to handle missing URLs without crashing.

- Implemented a better method of loading the BetterDiscord ASAR on Linux, via relative paths rather than hardcoding absolute paths. This makes the config portable between Native, Flatpak and Snap installations, and means that it won't break anymore if the user's home-folder is renamed or if they move the data to another computer/account.

- Added support for installing BetterDiscord's ASAR into multiple folders at the same time (via the "bdFolders" variable that we now build). This new feature is used for the Linux Flatpak/Snap support, since we need to install BetterDiscord separately into their sandboxes.

- The ASAR downloading process is now 2-3x faster, since it now saves it to a single, temporary file, and then copies that file into *all* Discord target paths (all Discord versions), so that it doesn't waste time downloading the exact same file over and over again for all destinations whenever the user has multiple Discord versions/paths on their system.

- The code for migrating old BetterDiscord installations has been rewritten to support all platforms, not just Macs. This makes it easy to do future moves of the BD data folders on any platform.

- Improved the GUI's logging messages, with better explanations instead of technical words such as "shims", since non-technical users are the primary audience of BetterDiscord-Installer.

- Added detailed comments for future maintainers/contributors.
- Now uses the unified function to "find latest Discord version", which solves the previous issues of having differing implementations everywhere.

- The "deleteAppDirs" function has been disabled on Linux, since it's completely irrelevant there. And its logging on Mac/Windows has been revised so that it doesn't output anything if there's nothing to delete.

- The "deleteModuleDirs" function has been rewritten to support Linux, and to support Flatpaks and Snaps. Its logging has also been revised for all platforms so that it doesn't output anything if there's nothing to delete.

- The "showInstallNotice" function has been completely rewritten to handle the fact that Discord *must* be started manually by the user to re-download its latest modules before we're able to install BetterDiscord again. The old "Repair" code was broken since it deleted Discord's modules-folders and then attempted to install BetterDiscord into those deleted folders. The new code warns the user about the situation and provides instructions for what they have to do, and won't let them proceed with the installation of BetterDiscord again until it has verified that the user has launched Discord manually (by checking that the installaton paths exist again).

- Improved the popup message formatting, for greater clarity.

- Improved the GUI's logging messages, with better explanations such as "exiting" instead of "killing", since non-technical users are the primary audience of BetterDiscord-Installer.

- Added detailed comments for future maintainers/contributors.
- Now uses clearer variable names and improved code formatting.

- Improved the GUI's logging messages, with better explanations such as "restarting" instead of "killing", since non-technical users are the primary audience of BetterDiscord-Installer.

- Added detailed comments for future maintainers/contributors.
- Semantic versioning. Increments the minor version to signify all the new features.
@Technetium1
Copy link

You just made a lot of people happy. Thanks Bananaman!

@SmolAlli
Copy link

SmolAlli commented Aug 23, 2022

What's with the 100 issues that all say the same 5 or so things? Is there a reason for that?
Also, there seem to be a lot of changes that are separate to what you have described. This will probably be questioned.

@Arcitec
Copy link
Author

Arcitec commented Aug 23, 2022

@Technetium1

You just made a lot of people happy. Thanks Bananaman!

Thanks. I am happy to help restore this installer for Linux. A lot of people want an easy GUI installer.

@SmolAlli

What's with the 100 issues that all say the same 5 or so things? Is there a reason for that?

Yes. It is necessary for three reasons.

  1. It ensures that those issues are automatically closed by GitHub when this is merged.
  2. It notifies every person who has had those issues, that a fix is on the way.
  3. It creates a two-way link for people who read those tickets, so that they can see the link to this resolution.

Also, there seem to be a lot of changes that are separate to what you have described. This will probably be questioned.

Every change is mentioned in the commit messages.

@SmolAlli
Copy link

It ensures that those issues are automatically closed by GitHub when this is merged.

Most of them were closed already though. Why not just do the open ones?

@Arcitec
Copy link
Author

Arcitec commented Aug 24, 2022

Most of them were closed already though. Why not just do the open ones?

It's just a courtesy to the people who had their tickets closed due to being "duplicates". It's a small amount of noise in the grand scheme of GitHub things, and it helps people who otherwise wouldn't have known about it. For example, if they've quit using BetterDiscord because their ticket was closed and they could never install it, and so on. If they've never interacted with the "main" ticket for each issue, then they would never know.

@kaosine
Copy link

kaosine commented Aug 25, 2022

If this works with the flatpak canary as it says I'm all for it. I've partially moved to flatpaks to help keep my system tree clean. So any support for that version helps ^_^

@kaosine
Copy link

kaosine commented Aug 27, 2022

As an update to my last comment, I noticed there was a build on their side and tested it. It now works with canary using flatpak. So that's a plus. Still need to test it more, but I'm pretty happy now that it works.

@Arcitec
Copy link
Author

Arcitec commented Aug 27, 2022

As an update to my last comment, I noticed there was a build on their side and tested it. It now works with canary using flatpak. So that's a plus. Still need to test it more, but I'm pretty happy now that it works.

Yep, I have extensively tested it with all versions listed in the long "checkmark ✅ table" in the original post. But it's still great to hear your results too. I am happy to have helped! :)

I provided that build exactly since I expect it to take a long time to review all the code changes and merge them. So if anyone else wants the new Linux build, it's above in the original post (as you found out). ❤️

@zerebos
Copy link
Member

zerebos commented Aug 31, 2022

Has this been tested with the current BetterDiscord asar? My understanding is that further changes are needed on that end to properly support snap (and other) installs due to configuration file locations.

@kaosine
Copy link

kaosine commented Aug 31, 2022

If you look at their chart it says it does. Whether or not that's true is another story. I just know it works with flatpak canary for me since I downloaded a build from their fork and used that to get it working on my end with flatpaks. Your mileage may very but they supposedly tested all versions so idk.

Copy link
Member

@zerebos zerebos left a comment

Choose a reason for hiding this comment

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

I've made some comments in the review that apply more globally across the review. Below are some more general thoughts.

By doing checks for process.platform === "linux" we are then explicitly not supporting BSD then?

Could this all be handled a bit cleaner if there were changes in BetterDiscord itself? Feel free to reach out to me on Discord to discuss. I am open to changed in the BD client to add first-class support for snap/flatpak installs, especially if it makes this process easier.

package.json Show resolved Hide resolved
},
"//": [
Copy link
Member

Choose a reason for hiding this comment

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

Remove

Copy link
Author

@Arcitec Arcitec Sep 2, 2022

Choose a reason for hiding this comment

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

Are you absolutely 100% sure about that? This is just a comment. It does nothing except tell all contributors: "Don't attempt to update the Electron version unless you also do the huge job of rewriting the entire application to stop using remote. calls." It's a good thing to have this comment, since people who see the ancient Electron 13 version will be immediately tempted to upgrade it. But they can't do that. The comment tells them that, and lets them know what must change if they want to port it to a newer version of Electron. I'll remove it if you are totally sure though.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I'm 100% certain about this, developer comments belong in better locations.

@@ -3,13 +3,15 @@ import phin from "phin";
const semverGreaterThan = require("semver/functions/gt");
const {version} = require("../../package.json");


Copy link
Member

Choose a reason for hiding this comment

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

Please remove unnecessary additional newlines

Copy link
Author

@Arcitec Arcitec Sep 2, 2022

Choose a reason for hiding this comment

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

The dual newlines between functions was added for my own sanity as a contributor. Dual newlines between functions are very common in programming, to make large files easier to read, and to provide clear breaks between every function. Only having single line-breaks creates very suffocating, dense code which isn't very breathable or readable. Remember that your code is meant to be readable by every contributor, and this change helps that. The easier it is for a new contributor to read the code and figure it out, the better. There were already plenty of places in the original, unmodified code which used dual newlines. I would like to formally request that dual newlines between functions stays. If that is okay with you. :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is not up to contributors to determine the styleguide of a codebase. Plus doing both mass style and functional changes in one PR is out of scope imo.

src/renderer/actions/install.js Show resolved Hide resolved
// After: "/home/foo/.var/app/com.discordapp.Discord/config/BetterDiscord".
// NOTE: Installation paths on Linux always contain those 4 suffix parts.
// NOTE: The requirement that BetterDiscord is installed in xdg-config
// is hardcoded into betterdiscord.asar, so we can't change these paths.
Copy link
Member

Choose a reason for hiding this comment

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

Well BetterDiscord itself (and the corresponding asar) can be changed in future releases if it would make things like this less janky. This feels very breakable.

Copy link
Author

@Arcitec Arcitec Sep 2, 2022

Choose a reason for hiding this comment

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

I would love to work together on that. The best solution would be if BetterDiscord scans a subdirectory of the currently running Discord's config directory. Perhaps by checking both the old (~/.config/BetterDiscord) and new paths, and preferring the new one, which would be "Directly under the parent Discord version's config directory", like this:

~/.config/discord/BetterDiscord.

Having such a structure would allow multiple native versions of Discord to co-exist. If a person installs Canary and Stable and PTB all on the same machine, they wouldn't overwrite each other's configs if all 3 are running at the same time.

Currently, Snap and Flatpak versions don't have that issue, since those are already sandboxed into individual config directories, thanks to each of them having their own virtual filesystems.

As for whether the current code you're commenting on is janky or breakable: It's not, because the installation path is determined by us in the same script: BetterDiscord itself searches for its own config in the config-directory, which is always 4 steps above the Discord path. We're the ones writing BetterDiscord into a path that is exactly 4 steps above the Discord path. So it's totally reliable. It's just a relative path to avoid having hardcoded paths (since the OLD hardcoded paths WERE very breakable).

Copy link
Member

Choose a reason for hiding this comment

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

Please reach out, I would like to learn the best path forward on this.

src/renderer/actions/uninstall.js Show resolved Hide resolved
const killErr = await kill(channels, (RESTART_DISCORD_PROGRESS - progress.value) / channels.length);
if (killErr) showRestartNotice(); // No need to bail out
else log("✅ Discord restarted");
if (killErr) showRestartNotice(); // No need to bail out if we failed, just tell user to restart manually.
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary extra space

Copy link
Author

Choose a reason for hiding this comment

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

It's a code-style that helps contributors visually see where the statement ends and the comment begins. It helps with skim-reading through a file, no matter if syntax highlighting of comments is a different color or not.

It's for example enshrined in the Python style-guides:

https://www.flake8rules.com/rules/E261.html

So I changed to it everywhere, but I can change back to the tight style if you prefer that. It would be no problem. I just did it to help out readability, for the same reason as the dual linebreaks between functions, which is another thing that really makes code a lot clearer for people who are not yet familiar with a codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Thankfully, this is not python and we do not abide by python style guides.

src/renderer/actions/utils/kill.js Show resolved Hide resolved
src/renderer/actions/utils/kill.js Show resolved Hide resolved
src/renderer/actions/install.js Show resolved Hide resolved
@Arcitec
Copy link
Author

Arcitec commented Sep 2, 2022

@rauenzi Hi Zack, thank you for reviewing the code and providing feedback. I will reply to each of your comments now and will take care of the changes within a week or so. I am really occupied at the moment, being there for a 12 year old who lives in an abusive home where he gets beaten by both of his parents, starved by his mother, and stays up all night to avoid being awake at day where he would get beaten. I am helping him get a way out through the police and his older brother. So my mind is very far away from programming right now. I'll get back to this work as soon as I am able.

 

Update: Thanks again for your very detailed review! I provided some feedback on some of the questions. I'll also contact you on Discord so that we can chat after this situation has calmed down, but I wasn't able to find you in the channels. You can send me a friend request. When I am ready to resume the work and implement the changes, we can talk and figure out if there are any questions to solve, such as the idea about making BetterDiscord's config path more flexible.

 

Update: I just noticed the extra comments outside the review. I'll reply to them below:

Has this been tested with the current BetterDiscord asar? My understanding is that further changes are needed on that end to properly support snap (and other) installs due to configuration file locations.

It was tested with all versions in the compatibility table as of the date I opened the pull request, and works perfectly. I tested every function marked with a ✅. Has anything changed in BetterDiscord.asar after that date?

Edit: Tested everything one more time today (Sept 14th, 2022), but nope, nothing has changed in BetterDiscord. Snap, Flatpak and Native all work as perfectly today as the day I opened the pull request. I even tried installing custom CSS Themes in the Snap versions (since you hinted that Snap doesn't support BetterDiscord), but it works perfectly! The original compatibility table in the initial message is still correct. :)

 

By doing checks for process.platform === "linux" we are then explicitly not supporting BSD then?

Yes, BSD doesn't have Snaps, doesn't have Flatpaks, and doesn't have any native Discord client (only web/unofficial clients).

We should definitely do something such as verifying that process.platform is one of win32, darwin or linux, and showing an "Unsupported operating system" error if the user has something else. I'll work on that!

Edit: I have now created an "Unsupported Platform" page which shows up if the user attempts to run the installer on an unsupported OS. It whitelists Windows, macOS and Linux. Those are the only operating systems that Discord supports, and which BetterDiscord and its installer support. Anything else will now just show the user an explanation about unsupported platforms.

 

Could this all be handled a bit cleaner if there were changes in BetterDiscord itself? Feel free to reach out to me on Discord to discuss. I am open to changed in the BD client to add first-class support for snap/flatpak installs, especially if it makes this process easier.

I'll see if there are other areas where things could be improved through changes to BetterDiscord itself. But right now the only thing that comes to mind is the change mentioned in the review: Making BetterDiscord on Linux prefer to live in a path underneath the current Discord version's config directory, to avoid file corruption whenever a person runs multiple native Discord clients (Discord, Canary and PTB). Currently they will corrupt each other since they all write to the same ~/.config/BetterDiscord path. For Flatpak and Snap there is no such issue since they use sandboxed environments, with one sandbox per installation, so each of them have their own unique BetterDiscord config directories. :)

 

Anyway, thanks a lot for the detailed review. Nice to see such brilliant people work on BetterDiscord! :)

Current Status of PR: Awaiting answers for the 4 open conversations above, before pushing the final version.

@NyllRE
Copy link

NyllRE commented Sep 18, 2022

when will this be merged?

@zerebos
Copy link
Member

zerebos commented Oct 28, 2022

Hi @Bananaman I seem to have lost our Discord DMs, can you send me another message on there?

@kaosine
Copy link

kaosine commented Jan 8, 2023

Any updates on this? I know I could just pull from their side on this, but it would be great to see this in the official release.....

@Etaash-mathamsetty
Copy link

Etaash-mathamsetty commented Apr 29, 2023

Bump, somebody pls fix the random code styling issues and merge this juicy boy

@iMonZ
Copy link

iMonZ commented Oct 16, 2023

Hey any news there?

@iMonZ
Copy link

iMonZ commented Oct 16, 2023

And any info about Wayland support?

@Hikiru
Copy link

Hikiru commented Feb 24, 2024

this still hasnt been merged?

@zerebos
Copy link
Member

zerebos commented Feb 24, 2024

Yes, I never received a followup DM and Bananaman seems to have gone inactive.

@NyllRE
Copy link

NyllRE commented Mar 21, 2024

ok now this is getting insane bro just solved a ton of issues and y'all left him on read for 2 years come on

@Hikiru
Copy link

Hikiru commented Mar 21, 2024

Yes, I never received a followup DM and Bananaman seems to have gone inactive.

Why do you need a follow up dm to merge it?

@zerebos
Copy link
Member

zerebos commented Mar 25, 2024

Why do you need a follow up dm to merge it?

There are still issues to fix and changes to make. #348 has a higher chance of being merged at this point as Sam has gone through and fixed some issues and we've discussed some changes that need to be made to improve things from the client side as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment