do NOT force discord as a dep on runtime#1131
Conversation
do NOT force discord as a dep on runtime
|
The DLL itself shouldn't be a problem either; it's less than 1 MB. Whatever you used to come up with this, the reasoning isn't right. |
I mean you're entitled to your opinion. It's really not a big deal, it just feels weird that the client expects a discord library to even boot in the first place. You're right that the package isn't that big and that you don't need discord itself to boot into the app, however I really don't see an issue with allowing people to remove this package, if they so want. The only known way to do this currently is to rebuild it from source and that's more of a hassle. But It's an opinion at the end of the day, and I understand and respect both sides of the scale. Not a necessary merge, just something that I think is a nice idea. The point is, if you don't have discord, it won't do anything so why gate the client, that's my reasoning. |
|
i mean.. the "define" check should works like leifa's said.. |
I have built this and it does work. Like Leifa said "It just won't do anything if you happen not to have it installed." So if it does nothing if you don't have discord installed, why not let the user delete the DLL? Gating a chatroom client behind a discord dependency is weird and bad UX but that's just my thoughts. By all means if someone wants to implement this a better way go ahead or just don't merge it. I offered an idea, you do you. |
|
okay, well.. my opinion (if may) are: again.. I just be neutral and I see nothing wrong with this either so.. 👍 pardon me if something of matter (yeah.. I just such an dumbs anyways) |
You're fine. It's an open discussion, for people to give their takes. |
The DLL is under 1 MB and does nothing if you don't use Discord, so leaving it there is harmless. Most users aren't going to start deleting DLLs just because they don't use Discord; they wouldn't even know what it's for. Also consider the complexity cost of adding this code just for the sake of being able to delete a DLL. Having it there is such a minor bother compared to the effort required to ensure it can be deleted. |
It's harmless but bad UX and locks the client behind an unnecessary dependencies. I sure as hell never use discord RPC, and I built my client without it and I know a few others who would appreciate that change. The code is fine, so I don't really see any downside whatsoever to even merge this. Having a dependencies for another social media site for an ao client doesn't sit right with me. You're right most users might not delete the DLL, but a few people might. The response I'm getting here is just "it doesn't do anything if you don't have discord", and that's the point!!! It does nothing so let people who want to, just remove it lol. The code is clean. I think unless other feedback is given, I have nothing more to say on the matter, or we'll be talking in circles. Dependencies should be actual dependencies, not optional features. |
|
"No downside" is wrong. You're adding like 80 lines and a guard on every Discord call to replace a guarantee the compiler gave you for free. All so people can delete a sub-1MB file that does nothing. We're just adding complexity to dodge a non-problem. |
Thanks for the feedback but I think you've misunderstood the implementation. I'm using QLibrary for dynamic loading and that isn't scattering runtime checks everywhere, it's a clean, self-contained solution. The code is 102 lines because it handles Windows, Linux, and AppImage paths properly, plus the function guards are just a single if (!ensure_discord_loaded()) return; at the start of each method. This is the standard way to make optional dependencies in Qt applications, and it enables packaging/distribution use cases without forcing users to rebuild. Again both are reasonable sides to be on but that's just what I think. |
|
Give me an actual reason beyond "I can delete one DLL file!" and the whole shipping part (again, 400kb saved...) that justifies the added codebase complexity (let alone the fact you'd just be introducing a 3rd way to disable Discord RPC entirely), and the PR might be worth a real look. |
It's not about the DLL. This is just about letting people who actually hate Discord truly get rid of it without rebuilding the whole thing. You're right in that, settings can disable it, but why carry a dead dll you don't need? Settings disable doesn't shrink the binary. If you're shipping this in a container or embedded system, you're still carrying Discord code. Dynamic loading means it's not compiled in at all. Open-source contributors don't need Discord RPC libraries installed to build and test locally. Lighter dependency chain easier onboarding for people wanting to hack on AO2. Also Some of us don't want Discord stalking our playtime lmao. Dynamic loading gives that freedom. I would much rather have an ao character taking up my hard drive instead of a stupid dependency that I never use, 102 lines well spent. I can keep putting more reasons but I think you get the point. |
|
Discord RPC is very rarely used, if ever, usually unintentionally considering what information its really there to offer. Most people turn it off, whether some people have no "need" for discord isn't exactly quantifiable, but the sentiment is there. That said, this is not really the way to deal with that issue. If it is unused and offers some sort of privacy issue, it should be considered for removal, not guarded against, and certainly not like this, this is considerably overkill. Removing discord RPC from a custom client is easily done (With some consideration towards 35gbs of QT), and as it is right now, it does not impact the user experience in any negative way and can always be toggled off, be that via client or discord itself. It is part of the client's features and therefore a "dependency" because the client does not function as intended without it. If people don't like it, then they can remove it themselves or not use it, that is an acceptable choice. In general, if we offered a "With discord" client and a "Without discord" client, it would solve the need for any of this. This should probably be discussed on some level when a rework to how AO2 works in general becomes a topic, but for now, this part can remain as a low priority issue. Thank you for your time and for bringing it up. I would recommend writing it up as an issue so it can be considered for a solution. This PR will not be merged. |
|
Damn, got told to write it as an issue instead lmao. Fair though ,separate builds is actually a cleaner solution than my dynamic loading proposal. And yeah, 35GB of Qt to remove a <1MB DLL, the true final boss of open-source Thanks for the thoughtful feedback. I'll file it and maybe revisit when the rework happens. Optional features should be optional dependencies. software engineering 101. |
|
Please do not add links to binaries in pull requests. |
Really not that big of a deal giving people alternatives if this an issue they personally have but sure, I'll keep that in mind. It was closed anyway and for people that didn't want to go into hell and build the client. The only reason I even made this an issue was I didn't realize discord was a dependency when building for the first time lol but yeah. Honestly, I don't get why I put this much time in if my prs are just going to get one upped, without giving me any feedback, or communicating with me at all whatsoever. It's toxic for an open source project and I don't really want to pour my energy here if that's all I'm going to get. I am mainly referencing my last PR not this one in particular. Not upset that things didn't get merged but we can do better as contributors and maintainers. But thanks for the feedback everyone. It's just how I have felt making PR's here the last few days. Other repositorys in the AO organization have been wonderful to work with like Akashi but so far this has been unpleasant having to wait weeks just for a review, only to find out somebody saw my PR, and just one upped it and theirs got merged the same day, instead of even communicating with me whatsoever is annoying. If you read this far into this, have a nice day. |
|
If you don't want new contributors just say so. Scooped. |
|
Not sure what exactly this is, but I can only imagine it is a built up of a bad experience. I apologize if you feel your time so far has been not well used, but the issues outlined here aren't breaking. Though the deletion of the info without prior warning is fair. They've been informed to kindly be careful in the future and not do that. It's still fair not to put direct links to outside sources though, this isn't really what a PR is for. Contributing code and then having it be later changed or outright removed or reconfigured is a natural part of open-source. The long-wait for reviews is due to a lack of hands, I've tried what I can and I further apologize if weeks is too much. It probably is, but given that some have waited years only now to be addressed, I hope you'll understand it certainly wasn't to single any of your works out, be it here or any other part of the org. We are welcoming any and all contributions. However, they will be put under some degree of oversight, with hopefully as little friction as possible. This may include favoring one thing over the other, though in prior cases, you have yourself chosen to have it occur with an approval of your own, I can't really give the benefit beyond that on why it was some bad idea. Nor can anyone tell you how to feel about it. Thank you for your efforts thus far, even if something isn't immediately resolved, every step helps, be that a PR or an issue that lays dormant for months until someone comes up with a suitable solution. I don't know what "Scooped" means. Please stop necro'ing this, this goes for all parties. |
Thanks for responding however If you genuinely don't know what 'scooped' means, look at PR #1124 vs #1129. That's the definition. You not knowing tells me everything I need to know. Thanks for the gaslighting. I'm done here. Have a nice day. |
|
Alright. I'm going to guess it means something being overtaken over something else that came before it. Not sure what in any of that was gaslighting, but it is your own works in the end. So its your call. I appreciate the provided help so far regardless. You have a nice day yourself. |
4 participants
do NOT force discord as a dep on runtime. I have build tested this.
Why this is bad? Some people have no need for discord or even use the feature, and we shouldn't have to make people rebuild the client from scratch just to remove that.
Before: The client would force discord to be a dep and refuse to boot without it.
After: It will boot whether discord rich presence is present or not