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

Implement Parallel Asynchronous Queue #100

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

formeo14
Copy link

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:

  1. HttpClient Management:

    • Replaced a outdated WebClient with HttpClient.
    • Ensured that one HttpClient instance is managed efficiently to avoid the pitfalls of repeatedly creating new instances.
  2. Parallel Asynchronous Queue:

    • Implemented the parallel asynchronous queue to manage downloads more effectively.
    • Utilized the Shard.DownloadAssistant library for this purpose, porting the DownloadManager class to integrate seamlessly.
  3. Project Upgrade:

    • Upgraded the project to .NET 8 to leverage the latest features and improvements and to use the package.
  4. Parallelism Configuration:

    • Hardcoded the parallelism settings within the codebase.
    • To modify these settings via the UI, a textbox would need to be implemented.
  5. Stop Button Functionality:

    • Disabled the stop button to always initiate new downloads.
    • If a stop button is required, additional UI implementation will be necessary.
  6. Cancellation Support:

    • Integrated direct cancellation functionality, similar to the solution proposed in Issue #53.
    • However, no UI element has been added to trigger this feature.
  7. Partial File Handling:

    • Retained the existing behavior for not handling partially downloaded files.
  8. Download All in Search:

    • The infrastructure is now in place to easily implement a "download all" feature in the search functionality, as discussed in Issue #3.
    • No UI element has been added for this feature.
  9. No Multiple Login Tokens:

    • The current implementation does not support 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.

@DJDoubleD
Copy link
Owner

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.
Or, a separate (non modal?) queue screen would need to be added.
Ideally, the queue would also need to have some minimal controls like removing items and clearing it, through the UI.

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).
I'm also unsure of the impact of the parallelism would have on the log files, that some people do rely on.

Anyway, I'm sorry for not responding sooner and hope you understand my reasoning.
Should you find a good implementation including the UI elements, I would be happy to take another look.

@formeo14
Copy link
Author

formeo14 commented Oct 12, 2024

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 Request library. The changes worked well in my tests, and I was satisfied with the results. The download speeds were fast, and it could handle multiple files efficiently while maintaining a bit messy but still the logs in the console.

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 RequestHandler.CancelMainCTS(); and buttons for start/stop that call _requests.Pause() or _requests.Start() in the main form. However, I struggled with finding a solution for the download speed, but the developer of the library recently introduced a new feature in the latest version. By changing the request container to ExtendedContainer<TrackRequest>, it's now possible to report the speed for all requests:

_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 IsDownload and setting RequestHandler.MainRequestHandlers[0].StaticDegreeOfParallelism = 1; to ensure that only one download runs at a time. This way, the queue functionality remains, but the parallelism is disabled. And the library could handle the retries and completion of partially downloaded files in the future, but I did not implemented this because as it is now was enough for me.

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., stop ...). This is where the UI becomes the biggest challenge, as you mentioned. Figuring out how to redesign the UI to manage these functionalities and without breaking existing log management for users who rely on it is indeed a complex task.

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
@formeo14
Copy link
Author

formeo14 commented Oct 20, 2024

Reopened Pull Request: Windows Forms Application Refactoring and Enhancement

I made further changes to the application as outlined below.

Key Changes:

  1. HttpClient Management:

    • Replaced outdated WebClient with HttpClient.
    • Ensured efficient management of a single HttpClient instance to avoid common pitfalls.
  2. Parallel Asynchronous Queue:

    • Implemented a parallel asynchronous queue to enhance download management.
    • Utilized the Shard.DownloadAssistant library, integrating the DownloadManager class seamlessly.
  3. Project Upgrade:

    • Upgraded the project to .NET 8 to leverage the latest features and improvements.
  4. Parallelism Configuration:

    • Hardcoded parallelism settings within the codebase.
    • To modify these settings via the UI, a textbox implementation would be required.
  5. Stop Button Functionality:

    • Implemented the stop button functionality to allow users to halt ongoing downloads.
    • Added a UI element to trigger the stop of all or a specific download.
  6. Cancellation Support:

    • Integrated direct cancellation functionality, similar to the solution proposed in Issue #53.
    • Added a UI element to trigger the cancellation feature.
  7. Partial File Handling:

    • Retained the existing behavior for not handling partially downloaded files.
  8. Download All in Search:

    • The infrastructure is now in place to easily implement a "download all" feature in the search functionality, as discussed in Issue #3.
    • Added a UI element to trigger the "download all" feature.
  9. No Multiple Login Tokens:

    • The current implementation does not support multiple login tokens.
  10. Spamming the Output:

    • Changed the way the parallel queue loads new albums for playlists, labels, and artists to reduce output spam.
  11. SpeedReporter:

    • The SpeedReporter now reports the speed of all parallel downloads every second.

Resolved Issues:

Preview:

Screenshot

@DJDoubleD
Copy link
Owner

Thank you for the update!
I will try to pull it onto a test branch as soon as I find some time to review and test it (might take me some time, apologies in advance).

Just from quickly looking through the code online, I do have a fairly simple naming & scope question.
Roughly speaking, you replaced the DownloadManager with the TrackRequest + TrackRequestOptions and moved those into the models package.
But the TrackRequest class/object isn't really a model (it doesn't map to anything, has state and behavior) and still seems like a utility object.
It also handles all types of DownloadItems that the DownloadManager did, so not only Tracks.
At first glance, the main difference between my implementation of the DownloadManager and your implementation of the TrackRequest, is that there was only 1 DownloadManager that was in charge of the entire download job and in your updated version, there are multiple TrackRequest that can be Queued (and extend Request).

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.

@formeo14
Copy link
Author

Thank you for taking the time to review the changes and for your feedback.

Regarding the naming and scope of TrackRequest and TrackRequestOptions, I see your point about the potential confusion with the term "model." You're right that TrackRequest doesn't strictly map to a data model and does indeed have state and behavior, which aligns more with a utility or service object.
The decision to move TrackRequest and TrackRequestOptions into the models package was primarily to encapsulate all related download logic within a single folder, but I agree that the current naming might be misleading. Your suggestion to rename them to DownloadRequest and DownloadRequestOptions makes a more sense.

I'll go ahead and update the names and package location accordingly. If you have any other suggestions or concerns, please let me know.

@DJDoubleD
Copy link
Owner

So I finally found some time to check out your PR version and test it.
It seems like the queued and parallel downloads work well (although for me parallel downloads don't seem to provide higher overall download speeds).

There are, however, to many bugs/missing features to be able to merge the changes into the master branch.
Here are some of the issues I've found:

  • There is no longer an assembly info to pull the app's package data from for e.g. the version number.
    This is probably due to the upgrade to .net 8, but I can't readily find a replacement.
    The version number is needed for the update check to work.
  • The UI seems to have been enlarged and elements spaced out a little, but this change is not consistently applied to all windows and especially in the main window, the "show/hide" feature of the settings panel, no longer works (settings are always open).
  • The buttons to open the logs & download folders, no longer work
  • The stop download button doesn't seem to fully work. It now seems to be dependent on the text that's entered in the url textbox, which will be a pretty steep hurdle to overcome for most users.
  • When a queue is cancelled (enter "cancel" in url textbox and click stop button), the queue doesn't seems to be cleared.
    If you try to download an item that was in the canceled queue, the app will not download the item, stating it's already in the queue
  • There is no visual indication of which items are in the queue
  • For some reason, the background image in the about form is not scaled anymore and shows clipping.
  • In the logging output, there are duplicate entries when starting the download (2 log lines are created, stating the download is starting in different phrasing)
  • The download speed indicator seems to give (for me at least) highly inaccurate readings and the 1 second interval seems way to high.

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 ideally (assuming all listed issue can be fixed), some kind of queue panel would need to be added to the UI, so the user gets a graphical overview of the items that are in the queue. Each item could then have separate controls to e.g. remove it from the queue and the panel could have a global "clear" button.
As I do lack a little creativity in designing good UI, I'm not completely sure how to best make these changes so the app remains simple and clear in usage, but also stays responsive (rendering a large queue, might be an issue using winforms).
I have since long been thinking of moving the tagging (and other) settings to a separate window, like I did for the app_id and secret. Perhaps it's time to do that to clear up more space in the main window (when I find some more time to do so...).

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.
I hope you understand my view and if you are willing to work on these issues, I would of course be more than happy to look at any future developments.

@formeo14
Copy link
Author

formeo14 commented Nov 4, 2024

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:

  • Assembly Information: I apologize for the oversight. The assembly contained incorrect .NET Framework information that blocked the version. It should be working now.

  • UI Enlargement: The UI seems to be enlarged due to .NET 8. I'm not sure why this is happening yet. But it works, although. I fixed the Tag view issue.

  • Explorer Reference: Yes, that's correct. It's not complicated to fix; the Explorer needs to be referenced. I fixed it.

  • Download Stoppage: This is by design, but it stops all downloads if no URL is provided.

  • Queue Clearing: The queue is cleared by canceling all requests, which are then automatically removed if the queue tries to start them. The issue you encountered was due to me forgetting to clear the container that holds the requests for management.

  • Visual Queue: There is no visual queue like in Deemix, and this PR does not provide or plan to provide that feature.

  • About Info Panel: I don't see any changes in the About info panel. If you could explain what you mean in more detail, that would be helpful.

  • Speed Measurement: The speed is accurate. How did you measure it? I read the network interface and compared the speed against the provided speed. It might differ due to other Windows connections, and the speed is shown in bytes, not bits. Could that be the issue? If you want to lower the refresh rate, you can do that—it's just a value to change. What refresh rate would you prefer if not 1x per second? Every 2 seconds?

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
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 29.80 MBps vs 18.80 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 18.89 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 29.00 MBps vs 24.94 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 24.94 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 23.81 MBps vs 23.92 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 24.92 MBps vs 22.87 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 22.88 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 23.79 MBps vs 23.95 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 23.95 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 25.65 MBps vs 21.54 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 20.18 MBps vs 22.27 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 23.73 MBps vs 21.65 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 23.48 MBps vs 22.89 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 22.66 MBps vs 23.87 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 23.81 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 17.71 MBps vs 18.28 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 36.72 MBps vs 18.28 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 18.68 MBps vs 21.47 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 20.93 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 17.08 MBps vs 0.00 MBps
  • Interface: Intel[R] Wi-Fi 6E AX211 160MHz, Download Speed: 0.00 MBps vs 0.00 MBps

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.
@formeo14
Copy link
Author

formeo14 commented Nov 4, 2024

I'm not entirely sure why I'm doing this, but I had a few hours to spare and decided to experiment with a queue representation. The issue lies with the 'utility' object that was previously part of the DownloadManager. This object needs to be separated to represent a single download (album/song) and should expose the DownloadItemInformation to display the state of the download along with relevant details. Here's a picture to illustrate my try.

Screenshot Queue

@DJDoubleD
Copy link
Owner

I'm not entirely sure why I'm doing this

Probably because experimenting can be fun ;),

The issue lies with the 'utility' object that was previously part of the DownloadManager. This object needs to be separated to represent a single download (album/song) and should expose the DownloadItemInformation to display the state of the download along with relevant details.

Yes, this was kind of my initial plan with the DownloadItems and the DownloadManager.
At some point I was hoping to separate the DownloadItems enough so they might be queued. I never got to that part, though.

Here's a picture to illustrate my try.

That is actually very nicely done! I like that layout.

I see you linked to your PR in @ImAiiR's original project.
His new SPA layout is actually a better visual framework to easily be able to add a queue view.
Given the fact that he has resumed work on his project, my mod project will probably become obsolete soon (if it hasn't already).

Anyway, I do thank you for your work and inspiration!

@formeo14
Copy link
Author

formeo14 commented Nov 7, 2024

"Probably because experimenting can be fun ;),"

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.
As said in .Net Developer blog

Move to .NET 8+: The most forward-thinking option, though, would be to upgrade to .NET 8 or higher. The .NET 8+ environment is the future of (not only) WinForms Application development and provides the most robust support, especially when it comes to third-party control vendors.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants