Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Commit f2c07d1

Browse files
grokysgrokys
committed
Add lifecycle for GitHub pane pages.
- Remove `NavigationViewModel.RegisterDispose` and call `Dispose` on pages when they're removed from the history - Add `Activated` and `Deactivated` methods to `IPanePageViewModel` which are called when the page is moved to/away from in the navigator
1 parent 2a85875 commit f2c07d1

File tree

5 files changed

+180
-59
lines changed

5 files changed

+180
-59
lines changed

‎src/GitHub.App/ViewModels/GitHubPane/NavigationViewModel.cs‎

Lines changed: 15 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
using System;
2-
using System.Collections.Generic;
32
using System.Collections.Specialized;
43
using System.ComponentModel.Composition;
54
using System.Linq;
6-
using System.Reactive.Disposables;
75
using System.Reactive.Linq;
86
using GitHub.Extensions;
97
using ReactiveUI;
@@ -19,7 +17,6 @@ public class NavigationViewModel : ViewModelBase, INavigationViewModel
1917
{
2018
readonly ReactiveList<IPanePageViewModel> history;
2119
readonly ObservableAsPropertyHelper<IPanePageViewModel> content;
22-
Dictionary<IPanePageViewModel, CompositeDisposable> pageDispose;
2320
int index = -1;
2421

2522
/// <summary>
@@ -42,6 +39,13 @@ public NavigationViewModel()
4239
.StartWith((IPanePageViewModel)null)
4340
.ToProperty(this, x => x.Content);
4441

42+
this.WhenAnyValue(x => x.Content)
43+
.Buffer(2, 1)
44+
.Subscribe(x => {
45+
if (x[0] != null && history.Contains(x[0])) x[0].Deactivated();
46+
x[1]?.Activated();
47+
});
48+
4549
NavigateBack = ReactiveCommand.Create(pos.Select(x => x.Index > 0));
4650
NavigateBack.Subscribe(_ => Back());
4751
NavigateForward = ReactiveCommand.Create(pos.Select(x => x.Index < x.Count - 1));
@@ -72,13 +76,13 @@ public void NavigateTo(IPanePageViewModel page)
7276
{
7377
Guard.ArgumentNotNull(page, nameof(page));
7478

79+
history.Insert(index + 1, page);
80+
++Index;
81+
7582
if (index < history.Count - 1)
7683
{
7784
history.RemoveRange(index + 1, history.Count - (index + 1));
7885
}
79-
80-
history.Add(page);
81-
++Index;
8286
}
8387

8488
/// <inheritdoc/>
@@ -103,7 +107,7 @@ public bool Forward()
103107
public void Clear()
104108
{
105109
Index = -1;
106-
history.Clear();
110+
history.RemoveRange(0, history.Count);
107111
}
108112

109113
public int RemoveAll(IPanePageViewModel page)
@@ -113,31 +117,11 @@ public int RemoveAll(IPanePageViewModel page)
113117
return count;
114118
}
115119

116-
public void RegisterDispose(IPanePageViewModel page, IDisposable dispose)
117-
{
118-
if (pageDispose == null)
119-
{
120-
pageDispose = new Dictionary<IPanePageViewModel, CompositeDisposable>();
121-
}
122-
123-
CompositeDisposable item;
124-
125-
if (!pageDispose.TryGetValue(page, out item))
126-
{
127-
item = new CompositeDisposable();
128-
pageDispose.Add(page, item);
129-
}
130-
131-
item.Add(dispose);
132-
}
133-
134120
void BeforeItemAdded(IPanePageViewModel page)
135121
{
136122
if (page.CloseRequested != null && !history.Contains(page))
137123
{
138-
RegisterDispose(
139-
page,
140-
page.CloseRequested.Subscribe(_ => RemoveAll(page)));
124+
page.CloseRequested.Subscribe(_ => RemoveAll(page));
141125
}
142126
}
143127

@@ -150,13 +134,8 @@ void ItemRemoved(int removedIndex, IPanePageViewModel page)
150134

151135
if (!history.Contains(page))
152136
{
153-
CompositeDisposable dispose = null;
154-
155-
if (pageDispose?.TryGetValue(page, out dispose) == true)
156-
{
157-
dispose.Dispose();
158-
pageDispose.Remove(page);
159-
}
137+
if (Content == page) page.Deactivated();
138+
page.Dispose();
160139
}
161140
}
162141

@@ -177,16 +156,7 @@ void CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
177156
break;
178157

179158
case NotifyCollectionChangedAction.Reset:
180-
if (pageDispose != null)
181-
{
182-
foreach (var dispose in pageDispose.Values)
183-
{
184-
dispose.Dispose();
185-
}
186-
187-
pageDispose.Clear();
188-
}
189-
break;
159+
throw new NotSupportedException();
190160

191161
default:
192162
throw new NotImplementedException();

‎src/GitHub.App/ViewModels/GitHubPane/PanePageViewModelBase.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,16 @@ public string Title
6464
/// <inheritdoc/>
6565
public IObservable<Uri> NavigationRequested => navigate;
6666

67+
/// <inheritdoc/>
68+
public virtual void Activated()
69+
{
70+
}
71+
72+
/// <inheritdoc/>
73+
public virtual void Deactivated()
74+
{
75+
}
76+
6777
/// <inheritdoc/>
6878
public void Dispose()
6979
{

‎src/GitHub.Exports.Reactive/ViewModels/GitHubPane/INavigationViewModel.cs‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,6 @@ public interface INavigationViewModel : IViewModel
5757
/// <param name="page">The page view model.</param>
5858
void NavigateTo(IPanePageViewModel page);
5959

60-
/// <summary>
61-
/// Registers a resource for disposal when all instances of a page are removed from the
62-
/// history.
63-
/// </summary>
64-
/// <param name="page">The page.</param>
65-
/// <param name="dispose">The resource to dispose.</param>
66-
void RegisterDispose(IPanePageViewModel page, IDisposable dispose);
67-
6860
/// <summary>
6961
/// Removes all instances of a page from the history.
7062
/// </summary>

‎src/GitHub.Exports.Reactive/ViewModels/GitHubPane/IPanePageViewModel.cs‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ namespace GitHub.ViewModels.GitHubPane
77
/// <summary>
88
/// A view model that represents a page in the GitHub pane.
99
/// </summary>
10-
public interface IPanePageViewModel : IViewModel
10+
public interface IPanePageViewModel : IViewModel, IDisposable
1111
{
1212
/// <summary>
1313
/// Gets an exception representing an error state to display.
@@ -48,6 +48,16 @@ public interface IPanePageViewModel : IViewModel
4848
/// </summary>
4949
IObservable<Uri> NavigationRequested { get; }
5050

51+
/// <summary>
52+
/// Called when the page becomes the current page in the GitHub pane.
53+
/// </summary>
54+
void Activated();
55+
56+
/// <summary>
57+
/// Called when the page stops being the current page in the GitHub pane.
58+
/// </summary>
59+
void Deactivated();
60+
5161
/// <summary>
5262
/// Refreshes the view model.
5363
/// </summary>

‎test/UnitTests/GitHub.App/ViewModels/GitHubPane/NavigationViewModelTests.cs‎

Lines changed: 144 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
using System;
22
using System.Reactive;
3-
using System.Reactive.Disposables;
43
using System.Reactive.Subjects;
54
using GitHub.ViewModels.GitHubPane;
65
using NSubstitute;
@@ -133,10 +132,101 @@ public void ForwardCommandShouldBeEnabledOnNavigatingBack()
133132
Assert.False(target.NavigateBack.CanExecute(null));
134133
Assert.True(target.NavigateForward.CanExecute(null));
135134
}
135+
136+
[Fact]
137+
public void BackShouldCallActivatedOnNewPage()
138+
{
139+
var target = new NavigationViewModel();
140+
var first = CreatePage();
141+
var second = CreatePage();
142+
143+
target.NavigateTo(first);
144+
target.NavigateTo(second);
145+
146+
first.ClearReceivedCalls();
147+
target.Back();
148+
149+
first.Received(1).Activated();
150+
}
151+
152+
[Fact]
153+
public void BackShouldCallDeactivatedOnOldPage()
154+
{
155+
var target = new NavigationViewModel();
156+
var first = CreatePage();
157+
var second = CreatePage();
158+
159+
target.NavigateTo(first);
160+
target.NavigateTo(second);
161+
162+
second.ClearReceivedCalls();
163+
target.Back();
164+
165+
second.Received(1).Deactivated();
166+
}
167+
168+
[Fact]
169+
public void ForwardShouldCallActivatedOnNewPage()
170+
{
171+
var target = new NavigationViewModel();
172+
var first = CreatePage();
173+
var second = CreatePage();
174+
175+
target.NavigateTo(first);
176+
target.NavigateTo(second);
177+
target.Back();
178+
179+
second.ClearReceivedCalls();
180+
target.Forward();
181+
182+
second.Received(1).Activated();
183+
}
184+
185+
[Fact]
186+
public void ForwardShouldCallDeactivatedOnOldPage()
187+
{
188+
var target = new NavigationViewModel();
189+
var first = CreatePage();
190+
var second = CreatePage();
191+
192+
target.NavigateTo(first);
193+
target.NavigateTo(second);
194+
target.Back();
195+
196+
first.ClearReceivedCalls();
197+
target.Forward();
198+
199+
first.Received(1).Deactivated();
200+
}
136201
}
137202

138203
public class TheNavigateToMethod
139204
{
205+
[Fact]
206+
public void ShouldCallActivatedOnNewPage()
207+
{
208+
var target = new NavigationViewModel();
209+
var first = CreatePage();
210+
211+
target.NavigateTo(first);
212+
213+
first.Received(1).Activated();
214+
}
215+
216+
[Fact]
217+
public void ShouldCallDeactivatedOnOldPage()
218+
{
219+
var target = new NavigationViewModel();
220+
var first = CreatePage();
221+
var second = CreatePage();
222+
223+
target.NavigateTo(first);
224+
first.ClearReceivedCalls();
225+
target.NavigateTo(second);
226+
227+
first.Received(1).Deactivated();
228+
}
229+
140230
[Fact]
141231
public void CloseRequestedShouldRemovePage()
142232
{
@@ -153,6 +243,21 @@ public void CloseRequestedShouldRemovePage()
153243
Assert.Single(target.History);
154244
Assert.Same(first, target.History[0]);
155245
}
246+
247+
[Fact]
248+
public void NavigatingToExistingPageInForwardHistoryShouldNotDisposePage()
249+
{
250+
var target = new NavigationViewModel();
251+
var first = CreatePage();
252+
var second = CreatePage();
253+
254+
target.NavigateTo(first);
255+
target.NavigateTo(second);
256+
target.Back();
257+
target.NavigateTo(second);
258+
259+
second.DidNotReceive().Dispose();
260+
}
156261
}
157262

158263
public class TheClearMethod
@@ -174,18 +279,35 @@ public void ClearsTheContentAndHistory()
174279
}
175280

176281
[Fact]
177-
public void DisposesRegisteredResources()
282+
public void DisposesPages()
178283
{
179284
var target = new NavigationViewModel();
180285
var first = CreatePage();
181286
var disposed = false;
182287

288+
first.When(x => x.Dispose()).Do(_ => disposed = true);
289+
183290
target.NavigateTo(first);
184-
target.RegisterDispose(first, Disposable.Create(() => disposed = true));
185291
target.Clear();
186292

187293
Assert.True(disposed);
188294
}
295+
296+
[Fact]
297+
public void CallsDeactivatedAndThenDisposedOnPages()
298+
{
299+
var target = new NavigationViewModel();
300+
var first = CreatePage();
301+
302+
target.NavigateTo(first);
303+
target.Clear();
304+
305+
Received.InOrder(() =>
306+
{
307+
first.Deactivated();
308+
first.Dispose();
309+
});
310+
}
189311
}
190312

191313
public class TheRemoveMethod
@@ -256,18 +378,35 @@ public void RemovingOnlyItemWorks()
256378
}
257379

258380
[Fact]
259-
public void RemovingItemDisposesRegisteredResources()
381+
public void RemovingItemCallsDispose()
260382
{
261383
var target = new NavigationViewModel();
262384
var first = CreatePage();
263385
var disposed = false;
264386

387+
first.When(x => x.Dispose()).Do(_ => disposed = true);
388+
265389
target.NavigateTo(first);
266-
target.RegisterDispose(first, Disposable.Create(() => disposed = true));
267390
target.RemoveAll(first);
268391

269392
Assert.True(disposed);
270393
}
394+
395+
[Fact]
396+
public void CallsDeactivatedAndThenDisposedOnPages()
397+
{
398+
var target = new NavigationViewModel();
399+
var first = CreatePage();
400+
401+
target.NavigateTo(first);
402+
target.RemoveAll(first);
403+
404+
Received.InOrder(() =>
405+
{
406+
first.Deactivated();
407+
first.Dispose();
408+
});
409+
}
271410
}
272411

273412
static IPanePageViewModel CreatePage()

0 commit comments

Comments
 (0)