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

Rubberduck.UI folder C#6&7 update #3562

Merged
merged 34 commits into from Dec 14, 2017

Conversation

Projects
None yet
6 participants
@IvenBach
Copy link
Member

IvenBach commented Nov 23, 2017

The first of many.

Update for Rubberduck.UI folder.

@@ -52,7 +52,7 @@ public void OnConnection(object Application, ext_ConnectMode ConnectMode, object
{
try
{
if (Application is Microsoft.Vbe.Interop.VBE)
if (Application is VBE)
{
var vbe = (VBE) Application;

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

can be simplified to if (Application is VBE vbe), which eliminates the cast

@@ -182,6 +179,7 @@ private void child_Click(object sender, CommandBarButtonClickEventArgs e)
return;
}

//Logger.Debug("({0}) Executing click handler for menu item '{1}', hash code {2}", GetHashCode(), e.Control.Caption, e.Control.Target.GetHashCode());

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

why?

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017

Author Member

Forgot to remove 😊

@@ -12,8 +12,6 @@ public RefactorEncapsulateFieldCommandMenuItem(CommandBase command)

public override string Key => "RefactorMenu_EncapsulateField";
public override int DisplayOrder => (int)RefactoringsMenuItemDisplayOrder.EncapsulateField;
//public override Image Image { get { return Resources.AddProperty_5538_32; } }
//public override Image Mask { get { return Resources.AddProperty_5538_321_Mask; } }

This comment has been minimized.

@Vogel612
@@ -17,7 +17,7 @@ public class NavigateCommand : CommandBase, INavigateCommand
protected override void OnExecute(object parameter)
{
var param = parameter as NavigateCodeEventArgs;
if (param == null || param.QualifiedName.Component == null)
if (param?.QualifiedName.Component == null)

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

IIUC this is not quite equivalent, because if param is null then param?.QualifiedName is null, which can cause a NullReferenceException now. I think I'd keep it the old way here

{
Func<QualifiedSelection?, string, IExtractMethodModel> createMethodModel = (qs, code) =>
var extraction = new ExtractMethodExtraction();
// bug: access to disposed closure - todo: make ExtractMethodRefactoring request reparse like everyone else.

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

maybe put the todo in a separate line? And possibly remove it, because it's not a concern for this place, but the refactoring.Refactor()

@@ -36,7 +36,7 @@ public static string Convert(string value)
public static string ConvertBack(string value)
{
var tuple = _keys.SingleOrDefault(k => k.Item2 == value);
return tuple == null ? value : tuple.Item1.ToString();
return tuple?.Item1.ToString() ?? value;

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

similar concerns as with the previous refactoring involving null-coalescing. Item1 can be null this way....

This comment has been minimized.

@Hosch250

Hosch250 Nov 23, 2017

Member

If tuple?.Item1 is null, the whole expression stops.

return _isBusy ? Visibility.Hidden : Visibility.Visible;
}
}
public Visibility EmptyUIRefreshMessageVisibility => _isBusy ? Visibility.Hidden : Visibility.Visible;

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

should access the Property to be consistent with line 510

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017

Author Member

So converting both back to the way it was?

This comment has been minimized.

@Vogel612

Vogel612 Nov 29, 2017

Member

no. this would be => IsBusy ? [...]

@@ -24,6 +24,11 @@ public class InspectionSeverityImageSourceConverter : IValueConverter

public object Convert(object value, Type targetType, object parameter, CultureInfo culture)
{
if (value == null)
{
throw new ArgumentException("Value parameter must be non-null");

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

why bring the nukes out directly? Log an error and return a default. Don't blow it all to smithereens directly...

This comment has been minimized.

@IvenBach

IvenBach Nov 26, 2017

Author Member

Possible 'System.NullReferenceException was brought up. Thought this was the proper way of addressing that.

using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;

This comment has been minimized.

@Vogel612

Vogel612 Nov 23, 2017

Member

wow .... that's a lot of usings

@bclothier

This comment has been minimized.

Copy link
Contributor

bclothier commented Nov 23, 2017

Seems to me 17 files were ExtractMethod variety which is being worked upon which will create unnecessary conflicts. I'd request to stash those files from the PR.

@IvenBach

This comment has been minimized.

Copy link
Member Author

IvenBach commented Nov 26, 2017

Is there anything else I ought to revert?

IvenBach added some commits Nov 27, 2017

@@ -52,19 +52,17 @@ public void OnConnection(object Application, ext_ConnectMode ConnectMode, object
{
try
{
if (Application is Microsoft.Vbe.Interop.VBE)
if (Application is VBE vbe1)

This comment has been minimized.

@MDoerner

MDoerner Nov 29, 2017

Contributor

The prefix might be redundant, but it is telling us that we are dealing with a RCW and not with a class we control.

IvenBach added some commits Dec 12, 2017

@retailcoder retailcoder merged commit 8657b1c into rubberduck-vba:next Dec 14, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@IvenBach IvenBach deleted the IvenBach:Rubberduck.UI_C#6&7_Update branch Dec 14, 2017

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