Conversation
kduske
left a comment
kduske
left a comment
There was a problem hiding this comment.
Thanks for splitting this up! Reviewing the individual commits was much easier. I left a bunch of comments, please have a look. Hope I'm not too nitpicky...
* Fixed bad newline removal changes * Refactored `selectProfileByIndex` into `selectProfileByName`. This encapsulates more logic for finding a profile within the manager, rather than having CompilationDialog do the heavy-lifting. * Adjusted method order in CompilationProfileManager.h. * Simplify validity logic in `loadLastCompilationProfileName` by using a ternary. * Rename signal `compilationProfileUsed` to `compilationProfileStarted` to avoid the use of the vague word "used".
Not nitpicky at all, thanks for being thorough. Sorry about the newline changes, I think I need to adjust the formatter in CLion. The feedback has been addressed in the latest commit, happy to make any other changes before merge. 😄 |
|
Please create individual fixups for the changes I requested so that they get squashed into the correct commits. I'd like to keep the history nice and clean instead of putting a cleanup commit on top. |
|
Thank you for addressing my feedback, the changes look good! |
881e225 to
e6d749b
Compare
No problem and great! I have broken up the individual fixups as individual commits and removed the "PR cleanup" commit. |
Sorry, this isn't what I meant. I meant to reduce the total number of commits back down by creating fixup commits that we can then squash before merging. For example, the commit history now adds a The idea is to have the history reflect the "ideal" path to the outcome and not the steps that you did as a developer. Rather imagine if you had to redo the PR but with 20/20 hindsight. |
|
If you feel this is too much trouble to go through that is fine, for a PR as small as this it would also be OK to squash everything into one commit. But I'd prefer to have a nice history. |
|
Ah okay sorry about that! I get what you're after now. I'm happy to go back and rewrite the commits, I'll do that this evening. |
e6d749b to
e3fd695
Compare
|
@kduske I have completed the fixups in 4 clean commits. Note that I ended up just taking all of the changes and making 4 fresh commits based on the relevant hunks in each file, as some of the commits to be fixup'd were straddling multiple base commits. Regardless, the spirit of what you're asking for should be here. |
8f70953 to
9307574
Compare
|
@1000nettles To speed things up, I have pushed a bunch of fixups to make some changes:
Please take a look at my changes and test it a bit. I hope I haven't broken anything! I might push some additional tests later. |
9307574 to
8d484a5
Compare
|
I have pushed a preparation commit that adds a few tests for |
1000nettles
left a comment
1000nettles
left a comment
There was a problem hiding this comment.
The changes make sense and generally look good, other than the styling issue I mentioned!
With regards to "Don't clear the last used profile name if no item can be found since it cannot bother us.", this is fine but just FYI the old profile name will still be displayed in the disabled menu item:
|
|
||
| auto* addProfileButton = createBitmapButton("Add.svg", "Add profile"); | ||
| m_removeProfileButton = createBitmapButton("Remove.svg", "Remove the selected profile"); | ||
| addProfileButton->setObjectName("CompilationProfileManager_AddProfileButton"); |
There was a problem hiding this comment.
I believe these object name changes are causing issues with the styling of these buttons. The initial object name (I think) is "toolButton_borderless" and set in lib/TbUiLib/src/BitmapButton.cpp, which is used for styling purposes. Here's how the compilation dialog looks on my machine:
There was a problem hiding this comment.
Nice catch, I missed that. It should be fixed now.
dbecf6f to
fc93463
Compare
Yep, I actually like it this way because it gives the user a chance to understand why the Re-run" action is greyed out. They can go into their Compile dialog and check whether the profile is available. |
|
@1000nettles I think everything works now. Could you give this one last test, too? |
We want to write tests for some classes that want to load image resources, so we copy these resources to make them available.
fc93463 to
4aac2e9
Compare
Looks like the builds are failing. I also tried on my local machine and they're failing on the same issue. |
This allows us to use the object names to identify widgets in tests.
We will use this function a later commit to pre-select a compilation profile and then run it.
…gnal selectAndRunProfile can be used to re-run the last used compilation profile in a future commit. The signal is used to store the name of the last used compilation profile.
We want to add a menu item to re-run the last used compilation profile, so we need to remember the name of that profile. We use QSettings to persist this across restarts.
We add a new menu item to re-run the last used compilation profile based on the name that we stored. If no such profile can be found (because it has been renamed or deleted or we are now using a different game), then the menu item is disabled.
4aac2e9 to
2be3a3b
Compare
Yeah I missed an include. It's fixed now. |
|
@kduske nice - everything's looking good! The changes are working as expected, and the button styling is fixed. 🙂 |
|
Thank you for testing it again, and for your contribution! |
2 participants
This is the second PR for the "Re-run compilation..." menu item addition, but with incremental commits showcasing the individual features added. I'll copy and paste some of the relevant details from the previous PR. Original PR can be found here.
This PR addresses some of the discussion in #1971.
Something to note: functionality that's missing from your comment is "or the first compilation profile if no profile has been run yet". Determining what the first profile is and keeping that in sync added some complexity which (to me) felt overkill, and instead I went with the flow outlined below. If you'd like me to add this in though, I can probably find an elegant way to do it.