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

Don't Force Apply button active when no update selected #2429

Merged
merged 2 commits into from May 1, 2018

Conversation

@DasSkelett
Copy link
Member

commented Apr 26, 2018

Problem:

#2428 showed, that MarkAllUpdatesToolButton_Click() always sets ApplyToolButton.Enabled to true, even without changeset. This is a very edge case of a bug which should get fixed soon (the changeset shouldn't be empty at all), but nevertheless, it would be better if the button doesn't get activated manually.

Solution:

So I removed this ApplyToolButton.Enabled call, as it is unnecessary in addition, because ApplyToolButton gets always updated in ChangeSetUpdated() which again is called by UpdateChangeSetAndConflicts(), which then again is called by ModList_CellValueChanged(), in turn automatically called on any checkbox-changes...
Ouf
In short, ApplyToolButton.Enabled is called everywhere, anytime.

Furthermore I rearranged (and hopefully) improved the code for marking mods for updates.
Feedback welcome!

Removed some unnecessary calls to `ApplyToolButton.Enabled = true` as well as setting the update checkbox true a bunch of times.
`ApplyToolButton.Enabled` gets set to true automatically because setting update-checkbox checked calls `ModList_CellValueChanged()`.
@pjf pjf added the Pull request label Apr 26, 2018
@HebaruSan HebaruSan added the GUI label Apr 26, 2018
@politas

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

I can see the logic behind putting the loop in MarkAllUpdatesToolButton_Click and having the single mark for update code in MainModList. It makes the MarkModForUpdate method more reusable, and puts the logic of "Mark all available updates" where it arguably should be (attached to the button that the user clicks to do that).

On the other hand, it's looping inside a loop, which is inefficient. Do we need to loop across the entire gridview to find a specific identifier in ModList? Is there a way to directly access the correct row? Some kind of LINQ query?

I think it you want to put the "Mark all available updates" looping code into MainModList (where it may arguably belong as an action taken on the ModList) it would be ideal to have it as a separate method. Simply putting the loop from MarkAllUpdatesToolButton_Click into MainModList maintains the nested loop, so that would be redundant. Your new single loop method is much more efficient. If we were calling MarkModForUpdate from anywhere else, it would be an easy call. It doesn't seem rational to leave the old code there if it isn't used.

Is there a way to rescue MarkModForUpdate by removing its loop?

Let me be clear, I'm entirely cool with that answer being "no", I'm just spilling out what's going on in my brain. There are many times with this code where I find myself thinking that the AwesomeCodersWhoCameBefore must have had Reasons for the choices they made. Those Reasons aren't necessarily right, though. 😃

@DasSkelett

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2018

I'm not sure if I can follow you exactly.
You want to split MarkModsForUpdate to split up in two methods, one MarkAllModsForUpdate (containing the loop/ query) and oneMarkSingleModForUpdate (just selecting a single mod, maybe called inside MarkAllModsForUpdate ?
That may be a good idea.

@HebaruSan

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

There is a map from identifier => row, see MarkModForInstall:

DataGridViewRow row = mainModList.full_list_of_mod_rows[identifier];

That could be used to eliminate the loop in the old code if we do keep MarkModForUpdate.

@politas

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

So before your changes, it was split up . You've basically merged the two elements (Find all mods which have updates) fromMarkAllUpdatesToolButton_Click and (Change the update checkbox) from MarkModForUpdate. If we change MarkModForUpdate to get the relevant row using DataGridViewRow row = mainModList.full_list_of_mod_rows[identifier]; then we get rid of the inefficiency and keep the concept of a method doing a single conceptual task.

I'm assuming that refreshing the modlist will cause the status of the Apply button to change?

@DasSkelett

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2018

So like this now?
I dont't know what row.Cells[1] is DataGridViewCheckBoxCell is/was for, this just checks if the first cell of the row is a checkbox?

@politas

This comment has been minimized.

Copy link
Member

commented Apr 28, 2018

It's testing to see if the second column (UpdateCol) is a checkbox. You might want to check what makes the status of that column change. I would have thought that mod.HasUpdate includes the same checks, so it may be redundant.

SetUpgradeChecked has a comment, //Contract.Requires<ArgumentException>(row.Cells[1] is DataGridViewCheckBoxCell); So checking for that might be to avoid a type mismatch exception? I don't know why the Contract.Requires is commented out.

@DasSkelett

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2018

I got it:
When we initiate all the cells in ConstructModList(), the cell updating (= row.Cells[1]) gets either declared as

  • DataGridViewCheckboxCell and casted to DataGridViewCell if the mod is installed, or
  • DataGridViewTextBoxCell if the mod is not installed:
var updating = mod.IsInstallable() && mod.HasUpdate
    ? (DataGridViewCell) new DataGridViewCheckBoxCell() {
        Value = myChange == null ? false
            : myChange.ChangeType == GUIModChangeType.Update ? true
            : false
     } : new DataGridViewTextBoxCell() {
         Value    = "-"
     };

But because GUIMos.HasUpdate = registry.HasUpdate() which checks if the mod is installed, the type check should be redundant:

/// <summary>
/// Is the mod installed and does it have a newer version compatible with version
/// We can't update AD mods
/// </summary>
public static bool HasUpdate(this IRegistryQuerier querier, string identifier, KspVersionCriteria version)
{
    CkanModule newest_version;
    try
    {
        newest_version = querier.LatestAvailable(identifier, version);
    }
    catch (ModuleNotFoundKraken)
    {
        return false;
    }
    if (newest_version == null) return false;
    return !new List<string>(querier.InstalledDlls).Contains(identifier)
        && querier.IsInstalled(identifier, false)
        && newest_version.version.IsGreaterThan(querier.InstalledVersion(identifier));
}
@politas

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Cool. This should make for a good little speedup.

@politas politas changed the title Fix "Apply changes"-button gets enabled without actual changes on "Add available updates"-click Don't Force Apply button active when no update selected May 1, 2018
@politas politas merged commit 96dc5a3 into KSP-CKAN:master May 1, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
politas added a commit that referenced this pull request May 1, 2018
@politas politas removed the Pull request label May 1, 2018
@DasSkelett

This comment has been minimized.

Copy link
Member Author

commented May 1, 2018

Ah, forgot to remove the comments...
We'll, may the next one tumbling over those lines worry his brain...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.