haacked / seegit

SeeGit - The Git Repository Visualizer
MIT License
508 stars 107 forks source link

Caught IndexOutOfRangeException in NotifyPropertyChanged.cs #64

Closed meson800 closed 10 years ago

meson800 commented 10 years ago

Added a try-catch block to avoid an IndexOutOfRangeException when viewing a repo with one commit. Previously, SeeGit would crash if the user selected a repo with only a root commit.

Fixes #63

haacked commented 10 years ago

Hmm, what's throwing that exception originally? Seems like we should fix it there.

meson800 commented 10 years ago

The exception originates inside the GraphSharp library. Here is the shortened stack trace.

Source=GraphSharp
  StackTrace:
       at GraphSharp.Algorithms.Layout.Simple.Hierarchical.EfficientSugiyamaLayoutAlgorithm`3.Sweeping(Int32 startLayerIndex, Int32 endLayerIndex, Int32 step, Boolean enableSameMeasureOptimization, Boolean& changed, Int32& phase)
       at GraphSharp.Algorithms.Layout.Simple.Hierarchical.EfficientSugiyamaLayoutAlgorithm`3.DoCrossingMinimizations()
       at GraphSharp.Algorithms.Layout.Simple.Hierarchical.EfficientSugiyamaLayoutAlgorithm`3.InternalCompute()
       at GraphSharp.Algorithms.AlgorithmBase.Compute()
       at GraphSharp.Controls.GraphLayout`3.Layout(Boolean continueLayout)
       at GraphSharp.Controls.GraphLayout`3.Relayout()
       at GraphSharp.Controls.GraphLayout`3.OnRelayoutInduction(Boolean tryKeepControls)
       at GraphSharp.Controls.GraphLayout`3.Graph_PropertyChanged(DependencyObject obj, DependencyPropertyChangedEventArgs e)
       at System.Windows.DependencyObject.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
       at System.Windows.FrameworkElement.OnPropertyChanged(DependencyPropertyChangedEventArgs e)
       at System.Windows.DependencyObject.NotifyPropertyChange(DependencyPropertyChangedEventArgs args)
       at System.Windows.DependencyObject.UpdateEffectiveValue(EntryIndex entryIndex, DependencyProperty dp, PropertyMetadata metadata, EffectiveValueEntry oldEntry, EffectiveValueEntry& newEntry, Boolean coerceWithDeferredReference, Boolean coerceWithCurrentValue, OperationType operationType)
       at System.Windows.DependencyObject.InvalidateProperty(DependencyProperty dp, Boolean preserveCurrentValue)
       at System.Windows.Data.BindingExpressionBase.Invalidate(Boolean isASubPropertyChange)
       at System.Windows.Data.BindingExpression.TransferValue(Object newValue, Boolean isASubPropertyChange)
       at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView, Object newValue, Boolean isASubPropertyChange)
       at MS.Internal.Data.PropertyPathWorker.UpdateSourceValueState(Int32 k, ICollectionView collectionView)
       at MS.Internal.Data.ClrBindingWorker.OnSourcePropertyChanged(Object o, String propName)
       at System.Windows.WeakEventManager.ListenerList`1.DeliverEvent(Object sender, EventArgs e, Type managerType)
       at System.Windows.WeakEventManager.DeliverEventToList(Object sender, EventArgs args, ListenerList list)
       at System.ComponentModel.PropertyChangedEventManager.OnPropertyChanged(Object sender, PropertyChangedEventArgs args)
       at SeeGit.NotifyPropertyChanged.RaisePropertyChanged(String propertyName) in C:\SeeGit\SeeGitApp\Bases\NotifyPropertyChanged.cs:line 18
       at SeeGit.NotifyPropertyChanged.RaisePropertyChanged[T](Expression`1 propertyExpression) in C:\SeeGit\SeeGitApp\Bases\NotifyPropertyChanged.cs:line 27
       at SeeGit.MainWindowViewModel.set_Graph(RepositoryGraph value) in C:\SeeGit\SeeGitApp\ViewModels\MainWindowViewModel.cs:line 43
       at SeeGit.MainWindowViewModel.MonitorRepository(String repositoryWorkingPath) in C:\SeeGit\SeeGitApp\ViewModels\MainWindowViewModel.cs:line 72
       at SeeGit.MainWindow.OnChooseRepository(Object sender, RoutedEventArgs args) in C:\SeeGit\SeeGitApp\MainWindow.xaml.cs:line 36
meson800 commented 10 years ago

@Haacked I found the issue, as referenced here.

A Graph with one vertex crashes when using the EfficientSugiyama layout method. Initially, I thought that I could, inside RepositoryGraphBuilder.Graph(), set the layout method based on VertexCount.

However, this caused a crash when moving from a single-commit repo to a multiple-commit repo, as RaisePropertyChanged(() => LayoutAlgorithmType); is called before RaisePropertyChanged(() => Graph); is called. This means the layout changes before the graph content does, causing the crash.

I tried switching the order of the above calls, but the crash still occurs, so I came up with another solution.

So, I changed it so that the layout is always set to CompoundFDP (a graph layout that doesn't crash with 1 vertex), and the whole graph is drawn. Then, if the number of vertices is greater than one, it switches the graph back to EfficientSugiyama and redraws. This eliminates all crashes when switching back and forth between single- and multi-commit repos.

The only side effect of this is that, when switching between any two repos, the pre-switch repo is drawn for about half a second in the CompoundFDP layout, before the post-switch repo is drawn in the correct Sugiyama layout.

Is this a good solution, or is there a more efficient solution?

haacked commented 10 years ago

Wow, thanks for looking into this. Have you tried seeing if there're any updates to GraphSharp that fix the bug? otherwise, I like this solution for now. :)

haacked commented 10 years ago

thumbs-up-keanu