Conversation
| codesign --force --deep --sign - "$APP_DIR" | ||
| codesign --verify --deep --strict "$APP_DIR" |
There was a problem hiding this comment.
We don't have a way to codesign these binaries.
| PROJECT_PATH="$ROOT_DIR/$PROJECT_REL" | ||
| SOLUTION_PATH="$ROOT_DIR/CSAUSBTool.sln" | ||
| APP_NAME="CSAUSBTool" | ||
| BUNDLE_ID="com.csausbtool.desktop" |
There was a problem hiding this comment.
This is not the package scope.
|
@sikaxn - Can you help explain some of your rational and choices here? This to me strongly smells of AI without a lot of logic behind the reasoning. The OSX build script specifically was a big red flag. I'm not against using AI to help, but having logical and clear decisioning behind each choice is still a requirement. You've made some sweeping changes here in the functionality of the app that aren't really helpful from a users perspective.
I do like the re-design on the selector/tags highlight, that's much cleaner to show in the list view; but the tabbed setup also was quite similar to that of the rest of the software suites provided by vendors, and the tab workflow would be good to keep as is if you can incorporate it. Would appreciate some clarifications here, as with the current state/explanations I don't want to merge this as is. |
1 and 2. Downloading the software lists (Step 1) does not download all in the folder. Instead, it call a GitHub API (https://api.github.com/repos/JamieSinn/CSA-USB-Tool/contents/Lists) which list all the avaliable file on /Lists under the directory. It is Step 2 that fetch the latest json (based on year) which used populate the software list. Under current configuration it will do Step 1 and Step 2 automatically. So there is no need for user to physically click step 1 and 2 button (see gif on #167). The year get chosen isn't based on Yes this need an extra APi call but guaranteed to work.
Also i don't know if this is just me but the current GUI tool barely works. It would freeze when I click download only unfreeze after download done. |
|
To clarify what I meant by the auto-download, is that it will automatically download a file that potentially should not be downloaded (ie - someone incorrectly uploads/edits a file with the year FRC9999.json) - this will break the functionality of the app for everyone - instead of binding the default to the year. There's much more risk involved with this approach than using the current year, which is generally safest due to the annual repeating nature of the tools use. Could you explain a bit more on your reasoning/rationale behind the workflow changes? The change to use the github API isn't really clear as to why this was done, nor is the year selector clear. The freezing and unfreezing is based on the fact I use the main thread of the application to download the software. This was mildly intentionally done to prevent concurrent downloads without too much hassle. I'm not massively tied to this process though as it was done as a way to prevent some user error without massive complication. Tag vs Tab I don't really have a preference to; but given the workflow for this is primarily used by CSAs to prep files for a given year - I don't know how much value the tag use case brings as a default sort/filtering option. I'm happy to be wrong about this; as I ironically haven't really used my own tool recently. |
JamieSinn
left a comment
There was a problem hiding this comment.
I like the idea of the settings page quite a bit. Lets move to use a proper top-nav bar drop down item; and change the settings to use a proper .NET Native method for it.
I'll take a re-review after this is implemented.
There was a problem hiding this comment.
Please remove this, we should never be distributing batch/shell scripts as part of a launch parameter. This should just be part of the documentation, or better yet - a top nav bar menu dropdown to open the settings.
There was a problem hiding this comment.
There is a button to open setting. However, it can be set to hidden. (see photo from previous comment, top right corner)
If setting page is set to hidden, there have to be some ways to bring it back. There are three method to do this. Remove the corresponding key on json, type enable setting on window and use launch parameters.
There was a problem hiding this comment.
Curious - why add a solution file here? There is already one at the root? https://github.com/JamieSinn/CSA-USB-Tool/blob/main/CSAUSBTool.sln
There was a problem hiding this comment.
This isn't following C# best practices.
https://learn.microsoft.com/en-us/dotnet/core/extensions/configuration
We should be using the Microsoft.Extensions.Configuration native system for configuration; not writing a custom one.
|
I been quite busy for the past couple days as my team just done Shanghai Regional and there are some important wrapup I needs to do with our team. I will be looking into Microsoft.Extensions.Configuration and impliment it. Yes your suggestion is valid and it should be the way to do. Some clarification about settings page needed. Setting page can be designed to be hidden.
I will get rid of LaunchWithSettings.bat. |





Remake the Ui with a better workflow
UI nolonger stuck while downloading
add a macos build script for on macOS debugging