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

[WIP/RFC] Multi-Select and INI File System #102

Closed
wants to merge 12 commits into from

Conversation

techydude0713
Copy link
Contributor

TLDR; Added a download queue which allows multiple apps or themes to be downloaded and sent, along with a INI file list system.

This PR aims to allow multiple packages to be selected to either download or send to the Wii.
To accomplish this, part of the download and WiiLoad functions were rewritten to focus on this queue based system.
Main Menu with Queue
When an app/theme is added to the queue, it's background color will change.
BG Color

If the repo changes, the queue list stays the same, regardless of contents.

Additionally, based on a request from #83, I also added a way to download (or send) the entire repository in Options -> Download All from Repo.

Because of the new queue system, I also decided to implement a package system that uses INI files. (See Package Example.ini). It works in both CLI and GUI mode.

I included an example in this PR (Package Example.ini).

#### Syntax: ####
# ('packageName' -> Download/send package ZIP),
# ('~packageName~' -> Send DOL/ELF only)
## For arguments: ('~packageName~ = <arg1,arg2,...>')
# ('|str|' -> Install message (can be before or after download.))
# ('#' -> Comment), 
#################

@dhtdht020
Copy link
Owner

dhtdht020 commented May 19, 2023

Hello, first of all, thank you for submitting this draft for a long requested feature. It is clear to me that this took a good amount of time.
At first glance, this seems to be doing exactly what it needs to, but a deeper look prompts some questions.

My first concern is the way multi-selection is implemented. Qt's QListWidget supports multi-selection natively, with a friendly checkbox to the left, highlighting and all. I think it would be much more user friendly to use that instead of the custom selection system implemented here. The system implemented here also looks pretty clumbersome, adding several UI elements to the main view.

My second concern is regarding these INI files. I completely understand the general idea behind them, and how they may be useful for some (saving a queue... loading a queue), but, I do not see the wider need in such a system. I was always a person who advocated for including as many features as possible in software, but in the case of OSCDL I am a single maintainer and I do not have the resources to keep maintaining features that I do not see much of a need in. This could potentially be brought in as an add-on for a plugin system I have planned in the future. If you'd like, remove this system from this pull request and start a discussion about implementing this in issues, so we can put more thought into this. I prefer pull requests to be small and focused.

Another thing to note is coding style. Spaces are missing where they usually are, functions and variables are named in different casing than the rest of the software, among other things. This is important for the code to be easily readable, and I expect contributed code to look consistent with its surroundings. (or at least be written in accordance with PEP8)

My last one: I am not sure what's exactly causing this from a first glance in the code, but something seems to have regressed overall window rendering with this pull request (or at least broke the font), at least on Windows. I've attached a comparison between rendering of master and this branch.

image
image

I recommend potentially starting over and ensuring the pull request stays small, readable, focused, communicated, and minimize opportunities for existing functionality to regress. Thank you for your continued contribution.

I will note that it's 100% my fault for not including any contribution guidelines while simultenously having specific expectations for them, that's something I will have to take care of eventually. For the time being, communication is key.

@techydude0713
Copy link
Contributor Author

No problem, I understand.
I'll close this PR and make a simplified version without the INI.

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.

None yet

2 participants