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

Show conflict messages in status bar #2442

Merged
merged 1 commit into from Jun 4, 2018

Conversation

@HebaruSan
Copy link
Member

commented May 8, 2018

Problems

If you try to install two mods that conflict with one another, they'll be highlighted red, but there are no hints as to the cause if they're not on-screen.

If you manually install a mod, and then attempt to install or upgrade a mod in CKAN that conflicts with it, the mod is not highlighted red and there are no other clues as to what's going on, see KSP-CKAN/NetKAN#6533.

Some effort was made toward creating a popup for this in #2237, but it was not completed. Part of the challenge is that a popup can be disruptive and is not something we would want to happen every time the user clicks a checkbox.

Causes

Conflicts previously were used to mark rows red, abort the creation of a change set, and leave the Apply button disabled, but specific descriptive messages weren't shown.

The code that highlights conflicts relies on RelationshipResolver.ConflictList, which only checks conflicts between CkanModules and ignores DLLs and DLCs.

Changes

The changes in this pull request are somewhat high risk and should be subjected to close scrutiny. It's possible that I missed an important test case, so please try any obscure things you can think of and report problems!

Status bar shows conflict messages

Now if you select two conflicting mods for install, a short description of the conflict is shown in the status bar to explain why the rows are highlighted:

image

If more than one conflict is present, only one will be shown, because we stop looking once we find the first. If the displayed problem is fixed, then the other conflict will be shown. Hopefully this will be enough to help users figure out what to do.

Previously, clicking any row would clear the previous status bar message. This doesn't make sense anymore now that the status bar contains useful information about the current conflict. This is now removed, and instead the message is cleared if we are able to successfully calculate a change set without problems.

Conflicts with DLLs included in RelationshipResolver.ConflictList

SanityChecker is rewritten to throw a new BadRelationshipsKraken exception instead of InconsistentKraken. This new exception inherits from the old one for backwards compatibility. Where InconsistentKraken provided only a list of opaque error strings, the new exception provides machine-readable descriptions of the problems to allow calling code to react to them.

RelationshipResolver now always runs SanityChecker. (SanityChecker's exception is only passed along if not turned off in the resolver options, as before.) If any conflicts with DLLs and DLCs are found, these are added to the data structure that provides RelationshipResolver.ConflictList (by way of the new exception described above). This allows the conflicting row to be highlighted. A description of the conflict will be shown in the status bar as described above:

image

Small things

The ", cannot install both" suffix is removed from the messages for conflicts, for consistency with other places that already don't have this.

In the course of investigation, I found several places where we check variable is null. The is operator in C# dynamically checks types, not values, and from what I can tell it's better to check variable == null. This pull request makes that change.

The var keyword obscures code because it's harder to tell what type an object is and what we're allowed to call on it. This is replaced with types in several places.

The status bar tooltip is now set to the text it contains, in case some message is very long and some runtime displays such tooltips (I was not able to get these to display on Mono).

Some conflict-checking code in CkanModule was duplicative of logic in RelationshipDescriptor. Now the duplication is removed by having one function call the other.

A blank comment line was missing from the auto generated code in Main.Designer.cs. This is now restored, which hopefully will help prevent parsing problems in Visual Studio.

@dbent

This comment has been minimized.

Copy link
Member

commented May 17, 2018

I'll see if I can get to this in the next couple of days.

@dbent
dbent approved these changes May 23, 2018
@dbent

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Code looks good, although I haven't actually tested it yet. A few general remarks:

In the course of investigation, I found several places where we check variable is null. The is operator in C# dynamically checks types, not values, and from what I can tell it's better to check variable == null.

That used to be the case, C# 7.0 introduced a functional-style pattern matching syntax. The compiler is smart enough to recognize that an is check against a constant (like null) is just a equality check. obj is null wouldn't even be valid in C# versions prior to 7.0.

For confirmation you can check the IL produced by these two pieces of code:

object o = null;
if (o == null)
	Console.WriteLine();
object o = null;
if (o is null)
	Console.WriteLine();

In the first case you get:

IL_0000:  nop         
IL_0001:  ldnull      
IL_0002:  stloc.0     // o
IL_0003:  ldloc.0     // o
IL_0004:  ldnull      
IL_0005:  ceq         
IL_0007:  stloc.1     
IL_0008:  ldloc.1     
IL_0009:  brfalse.s   IL_0011
IL_000B:  call        System.Console.WriteLine
IL_0010:  nop         
IL_0011:  ret         

In the second case you get:

IL_0000:  nop         
IL_0001:  ldnull      
IL_0002:  stloc.0     // o
IL_0003:  ldloc.0     // o
IL_0004:  ldnull      
IL_0005:  ceq         
IL_0007:  stloc.1     
IL_0008:  ldloc.1     
IL_0009:  brfalse.s   IL_0011
IL_000B:  call        System.Console.WriteLine
IL_0010:  nop         
IL_0011:  ret    

They're identical. In both cases the two operands are simply loaded onto the stack (lines 3-4) and then compared using the ceq instruction (line 5), no dynamic type checking is performed.

The var keyword obscures code because it's harder to tell what type an object is and what we're allowed to call on it. This is replaced with types in several places.

I generally disagree, one of the benefits of static typing is that we can leverage tools like the IDE to aid the programmer. We know what type an object is because the compiler, and by extension the IDE, knows. We know what we're allowed to call on it because the compiler, and by extension the IDE, knows.

In general, the only time I wouldn't use var is if somehow the specific type of a reference is critically important and wouldn't be caught statically. For example, let's say you perform some huge set of LINQ statements that produce an IEnumerable<T>. Iterating over this would lead to multiple enumeration and cause all the LINQ statements to reexecute multiple times. Therefore to indicate that the enumerable should be preemptively enumerated for performance reasons you may declare the variable storing the result as List<T>, forcing a .ToList() at the end and preventing multiple enumeration. But even then, I'd opt to use the most basic type or interface that captures my intent (which is simply to prevent multiple enumeration), in which case I'd use ICollection<T>.

@HebaruSan

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

The compiler is smart enough to recognize that an is check against a constant (like null) is just a equality check.

Good to know it wasn't actually misbehaving previously, but I think == null still more clearly expresses the intent here, since it's the established idiom for checking null. The next programmer to read this code won't have to run an IL generation experiment to make sure. 😁

one of the benefits of static typing is that we can leverage tools like the IDE to aid the programmer.

That assumes that code is only ever viewed in an IDE with those specific features. Personally, I use a regular text editor, and I also frequently browse code on GitHub or in tools like Gitg. In such situations, var can require cross referencing extra files to understand what's happening. It's not that difficult to make the code welcoming of developers using a variety of environments. (I'm also not a fan of the idea that changing a return type in some far-away module will cause my function to start using a different type for a local variable (the most private scope possible), but I realize it would be possible to code intentionally to make that viable.)

The only time I find var non-obfuscating is when the variable is being assigned a new expression, so the type is already on the line explicitly once anyway. But certainly if a more generic interface is applicable, there's no need to declare the absolute most specific type.

@politas

This comment has been minimized.

Copy link
Member

commented May 27, 2018

Thanks, @dbent, I was hoping for exactly that sort of discussion!

In testing, I note that when the modlist is refreshed (removing all selections) thanks to changing KSP compatibility settings, the status bar message indicating conflicts does not disappear.

@HebaruSan HebaruSan force-pushed the HebaruSan:fix/status-bar-conflicts branch from e2261a9 to 17d9d77 May 29, 2018
@HebaruSan

This comment has been minimized.

Copy link
Member Author

commented May 29, 2018

@politas, thanks, I moved the status bar clearing from where it was to a spot that should be hit regardless of who resets the change set.

@politas politas merged commit 17d9d77 into KSP-CKAN:master Jun 4, 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 Jun 4, 2018
@HebaruSan HebaruSan deleted the HebaruSan:fix/status-bar-conflicts branch Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.