-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement Parallel Asynchronous Queue #100
base: master
Are you sure you want to change the base?
Conversation
I'm sorry I hadn't reacted to your PR before but I just wanted to let you know that I did see it and appreciate your effort and willingness to contribute! I just didn't have much time to work on the project and the main problem with your PR was that I didn't see a way to merge your implementation of the queue and parallel downloads (and the other changes), without also doing the required UI redesign to both retain all existing functionality (stopping/starting download, dl speed shown, ...) and add the minimum visual representation needed to show the user what is going on. The way I saw it, either the main screen would need to be redesigned so that the output text area becomes more of a console, a visual representation of the tracks being downloaded in parallel would need to take the center space and a queue list would need to be added to the side somewhere. Without the respective UI elements, I just expect to many questions/problems from users, as the simple addition of the settings screen for custom app_id and app_secret has demonstrated (I got a LOT of questions & "this doesn't work" comments about that). Anyway, I'm sorry for not responding sooner and hope you understand my reasoning. |
I completely understand your concerns and appreciate you taking the time to explain them. I didn't initially expect my PR to be merged, especially given the significant changes I made, such as updating to .NET 8. My intention wasn't to push for a major overhaul but rather to contribute something that could potentially improve the project. When I first encountered your project, the Deezer ARLs on FireHawk were under attack, and I wanted to test out the project's capabilities. I could only start downloads after another finished and I noticed an issue about parallel downloads and some that align with that. So I decided to spend an evening modifying a few classes to use the Regarding the existing functionalities like stopping/starting downloads and displaying download speeds, I initially thought it would be straightforward to implement. For example, adding a cancel button that calls _requests.SpeedReporter.SpeedChanged += (obj, speed) => {
Debug.WriteLine(speed);
UpdateDownloadSpeedLabel(speed);
}; Clearing the queue could be done with something like: _requests.Remove(_requests.ToArray()); Although this approach is a bit messy, it works. For clearing finished items, one could use: _requests.Remove(_requests.Where(req => req.State == RequestState.Completed).ToArray()); Additionally, if you would have preferd to disable parallelism while keeping the queue functionality intact, I considered removing any However, stopping specific requests would indeed require a more significant redesign to allow users to stop a specific request through a visual queue representation or a command interface (e.g., I could not find out how to change the UI effectively because I have no experience using Windows Forms. I'm more comfortable with a markup language for design rather than manually changing the forms code or using the Toolbox in the visual editor. In summary, I understand the complexities involved and appreciate your detailed feedback. |
Refactor logging to be download-specific
- Fix issue where _downloadInfo was being overwritten - Correct SpeedReporter to accurately report download speeds - Prevent excessive album output spamming
Reopened Pull Request: Windows Forms Application Refactoring and EnhancementI made further changes to the application as outlined below. Key Changes:
Resolved Issues:Preview: |
Thank you for the update! Just from quickly looking through the code online, I do have a fairly simple naming & scope question. Would it therefore not be more logical to rename the TrackRequest & TrackRequestOptions to e.g.: DownloadRequest & DownloadRequestOptions and move them back to the Shared package for clarity and consistency? Feel free to correct me if I'm missing something on this point. |
…and `DownloadRequestOptions`
Thank you for taking the time to review the changes and for your feedback. Regarding the naming and scope of I'll go ahead and update the names and package location accordingly. If you have any other suggestions or concerns, please let me know. |
So I finally found some time to check out your PR version and test it. There are, however, to many bugs/missing features to be able to merge the changes into the master branch.
I feel like, in order to fully incorporate the queue & parallelism changes, the UI needs to fully accommodate these features as the application is aimed at regular users that like having a clear UI as the CLI tools out there are too complex for them. So I feel like this PR is still more a "work in progress" that isn't really ready to be merged into the master branch and be released. |
Great to hear that the parallel downloads are working well for you. For me, they provided around double the download speed, although I didn't measure it precisely—just compared it to the Task Manager. However, it's easy to disable if needed, so you could consider adding a user setting to enable or disable this feature. Regarding your points:
These measurements are as accurate as I think is fitting. The discrepancies are due to different calculations and times, and I don't know what else the PC is requesting. Speed Comparison Data
The queue feature is nice, but I won't be adding it in this PR. I think it would take about 3 days to implement, and at the moment, I don't want to invest that time into WinForms and a system that works for me. You've found a few bugs that I can fix, but due to the update to .NET 8, this PR isn't ready to be merged without addressing the changes you don't like or that were made because of the update. Feel free to add commits to change it in a way you think it needs to be for merging. |
Corrected directory buttons functionality. Re-enabled Tag View expandability. Fixed log line to accurately reflect download start.
Probably because experimenting can be fun ;),
Yes, this was kind of my initial plan with the DownloadItems and the DownloadManager.
That is actually very nicely done! I like that layout. I see you linked to your PR in @ImAiiR's original project. Anyway, I do thank you for your work and inspiration! |
True, but WinForms isn't exactly enjoyable or ideal for design. As seen in @ImAiiR's project, the round borders look more modern, but WinForms turns everything into a pixelated mess. The older design seems higher of quality, while the newer ones appear more modern. But booth should be considered. The same issue arises with my queue representation; if there are 100 items, it doesn't virtualize, forcing me to dynamically load and unload images. Modern frameworks handle this automatically and are also compatible with other devices. So, it's generally a legacy framework, mainly used for legacy business software. It is not rely fun to do that by hand. I've created a first TrackRequest object, which anyone is free to build upon. I won't be creating anything further based on it, such as AlbumRequest or label.... The codebase for https://github.com/ImAiiR/QobuzDownloaderX is smaller, as is the API, though I'm unsure if that's a good or bad thing as it still relies on the outdated WebClient, and is not updated to .Net.
For me, it was a brief excursion into Qobuz, which was enjoyable, but I'm not rely a user as I can't really integrate into my pipeline: Spotify add to library -> Lidarr -> Deezer/Tidal plugin -> Lidarr -> Jellyfin -> Symfonium. |
Windows Forms Application Refactoring and Enhancement
I have identified several areas of improvement within the current Windows Forms application, which I believe could benefit from a more structured approach. My contributions have introduced additional complexity, albeit with the intention of enhancing functionality.
Key Changes:
HttpClient Management:
WebClient
withHttpClient
.HttpClient
instance is managed efficiently to avoid the pitfalls of repeatedly creating new instances.Parallel Asynchronous Queue:
Shard.DownloadAssistant
library for this purpose, porting theDownloadManager
class to integrate seamlessly.Project Upgrade:
Parallelism Configuration:
Stop Button Functionality:
Cancellation Support:
Partial File Handling:
Download All in Search:
No Multiple Login Tokens:
Resolved Issues:
These changes aim to address existing issues and lay the groundwork for future enhancements. Further UI adjustments and feature implementations may be required to fully realize the potential of these improvements.