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

Add username who made the commit - Issue #85 #86

Merged
merged 3 commits into from
Feb 6, 2017

Conversation

ben-m-lucas
Copy link
Contributor

Implementation for adding the username who made the commit

update adorner to start layout right-aligned to each commit
@haacked
Copy link
Owner

haacked commented Jan 30, 2017

Thanks! I'll try and look at this soon.

@haacked
Copy link
Owner

haacked commented Feb 1, 2017

One of the goals of SeeGit is to be a teaching tool during a live talk. In that scenario, the speaker is the only one making commits so the name is redundant.

My preference would be to add a setting to show the name and default it to be off. We already have other settings. If you make the setting sticky, it should accomplish what you want here.

@ben-m-lucas
Copy link
Contributor Author

ben-m-lucas commented Feb 3, 2017

That makes sense. I added an option in the settings for showing the Commit Author.

I did have one bit of code in the OnToggleSettings method that I'm not thrilled with. When the commit author is displayed, it changes the width of each CommitVertex. When this happens, a call to Relayout needs to be made. Ideally, the fact that the Graph property of the ViewModel changed should be enough information for the control to know that it needs to lay itself out again. I'm not familiar enough with the RepositoryGraphLayout control to do this though.

@@ -9,6 +9,7 @@ namespace SeeGit
/// </summary>
internal class CommitTemplatedAdorner : Adorner
{
private FrameworkElement _adornedElement;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but since this isn't settable, it should be readonly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just committed the change to use readonly.

@haacked
Copy link
Owner

haacked commented Feb 3, 2017

Maybe the Width property doesn't implement INotifyPropertyChanged properly. In any case, I'm happy with that trade-off. People don't change their settings all the time.

@haacked haacked merged commit 472ef49 into haacked:master Feb 6, 2017
@haacked
Copy link
Owner

haacked commented Feb 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants