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

Compare RelationshipDescriptors with Equals instead of Same #2735

Merged
merged 1 commit into from
Apr 21, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 18, 2019

Problem

@Poodmund reported an issue on the forum where this popup appears every time CKAN is launched, even after you click Yes to reinstall:

screenshot

https://forum.kerbalspaceprogram.com/index.php?/topic/154922-ckan-the-comprehensive-kerbal-archive-network-v1260-baikonur/&do=findComment&comment=3586115

Cause

At the top level, Repo.RelationshipsAreEquivalent uses RelationshipDescriptor.Same to compare the relationships of an installed module with those of its freshly updated available counterpart:

CKAN/Core/Net/Repo.cs

Lines 321 to 330 in 5331bf9

for (var i = 0; i < a.Count; i++)
{
var aRel = aSorted[i];
var bRel = bSorted[i];
if (!aRel.Same(bRel))
{
return false;
}
}

Child class ModuleRelationshipDescriptor implements Same as a comparison of the name and versions:

public override bool Same(RelationshipDescriptor other)
{
ModuleRelationshipDescriptor modRel = other as ModuleRelationshipDescriptor;
return modRel != null
&& name == modRel.name
&& version == modRel.version
&& min_version == modRel.min_version
&& max_version == modRel.max_version;
}

Child class AnyOfRelationshipDescriptor implements Same by comparing the lists contained in the any_of property using SequenceEqual:

public override bool Same(RelationshipDescriptor other)
{
AnyOfRelationshipDescriptor anyRel = other as AnyOfRelationshipDescriptor;
return anyRel != null
&& (any_of?.SequenceEqual(anyRel.any_of) ?? anyRel.any_of == null);
}

However, when SequenceEqual checks the pairs of values in the two lists, it uses Equals, not Same! This means the nested relationships aren't compared the same way as regular ones, so they might not produce the correct result. This causes CKAN to think an any_of relationship has changed when it hasn't, and mistakenly alert @Poodmund to it.

Changes

Now RelationshipDescriptor implements IEquatable<RelationshipDescriptor>, and it and its child classes implement Equals instead of Same, with the same logic as before. Calling code is updated to use Equals instead. This will ensure that the intended comparison logic is used when we call SequenceEqual.

Fixes @Poodmund's issue.

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Pull request Relationships Issues affecting depends, recommends, etc. labels Apr 18, 2019
@Olympic1 Olympic1 removed the Bug Something is not working as intended label Apr 18, 2019
@HebaruSan HebaruSan added the Bug Something is not working as intended label Apr 18, 2019
@Poodmund
Copy link

What do you need from me and how do I acquire such logs?

@HebaruSan
Copy link
Member Author

The first thing that would be helpful is a copy of your KSP/CKAN/registry.json file. There may be some quirk in your copy of the metadata that isn't present in mine. Beyond that, just filling in the issue template would be useful in answering such questions as game versions, etc.

@Poodmund
Copy link

Background

CKAN Version: 1.26.0

KSP Version: 1.6.1.2401

Operating System: Windows 10 64bit

Have you made any manual changes to your GameData folder (i.e., not via CKAN)? Yes, to install mods that are not listed on CKAN.

Problem

What steps did you take in CKAN? Opening CKAN for the first time each boot cycle.

What did you expect to happen? CKAN opens and automatically updates the repository list.

What happened instead? CKAN loads and hangs with a pop-up appearing stating that a mod has had its metadata changed since the last update and recommends reinstalling it the preserve consistency. If you click Yes or No in the pop-up, CKAN then takes the respective action, updates the repository list and loads into the mod listing screen as per expectations.

Screenshots: See above

CKAN error codes (if applicable): No error codes

Registry.json: https://www.dropbox.com/s/3icb9v3lms8q9e1/registry.json?dl=0

@HebaruSan
Copy link
Member Author

HebaruSan commented Apr 18, 2019

Thanks! Taking a look, the available module exactly matches the installed module, so that rules out a metadata quirk.

Maybe a platform thing; could be that SequenceEqual and Equal have different default behaviors on Windows and Mono (I'm in Linux right now).

This test build has the fix from this pull request, if you wouldn't mind trying it out and reporting whether the problem is fixed:

@Poodmund
Copy link

Testing the above build, CKAN hangs with most of the UI missing for about 5 seconds upon initial application opening then it kicks into gear and starts automatically updating the repo and then loads into the mod list.

The change seems to have resolved my issue and I have hard-cycled the PC 5 times to test and all 5 times CKAN booted into the mod list without showing the pop-up.

@Poodmund
Copy link

I don't know whether it is related, but trying to launch CKAN today just opens the initial console window, it closes after a second or so, then nothing else launches. Hovering over the window preview in the taskbar just shows a blank white preview of the CKAN window but it never opens.

I tried downgrading the executable to the 1.26.0 release but the same issue is occuring.

Could there be any possible relation?

@DasSkelett
Copy link
Member

Sounds like you encountered #2717 ?

@Poodmund
Copy link

Yep, exactly that, thanks for the heads up and sorry for the false alarm. Weird bug though.

@HebaruSan HebaruSan merged commit 139b3a3 into KSP-CKAN:master Apr 21, 2019
@Olympic1 Olympic1 removed Bug Something is not working as intended Pull request labels Apr 21, 2019
@HebaruSan HebaruSan deleted the fix/reldesc-same-equals branch April 21, 2019 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants