Feature. Quick View: Show history of file #573
Conversation
|
It is too slow now. It seems that my approach to gather history commits is incorrect. Statistics commits: 6011 Benchmarks
Improvements in UI What do you think? |
|
Maybe this PR also should contain option from #383 |
|
@lucasderraugh |
|
Running this from Xcode at the moment simply aborts in debug: |
|
Not sure what I'm expected to be seeing here, but everything I look at just seems to be empty space on the left. |
|
@lucasderraugh If latest commit fixed For example, if you take commit at master branch |
|
Your latest commit fixed the assertion, but I just don't see anything other than empty space. Every commit in the map view looks like the image I posted above when I hit the spacebar to go into one. |
|
@lucasderraugh |
|
Ok, well now I'm just hitting the |
|
@lucasderraugh |
|
Ok, just please state all the issues up front so I’m not debugging for myself. We can work through these but I have to know what doesn’t work beforehand. |
|
If I go to bca272f and use the file history on Debug.xcconfig, it would appear that it picks up commits that don't affect Debug.xcconfig. It picks up ccd76bb as a commit that changed that file, but it doesn't appear in the diff. I will finally have some time to myself this coming weekend, so I'll try and help debug what we can do here. Some side notes:
|
|
@lucasderraugh |
|
I don’t want a new window, just a different view perhaps. Though this can get confusing with having 2 layers deep of 1Up. |
|
@lucasderraugh Could we also move to NSLayoutConstraints instead of plain views and Autoresizing masks? |
|
@lucasderraugh Check it. |
|
@lucasderraugh Fixed finally, I hope... |
|
@lucasderraugh I suggest to add method to |
|
@lucasderraugh |
|
What's the current status of this PR? Were all the apparent issues addressed? |
|
Some issues that I have now
1. Slow fetching of commits for specific files. Not sure that this is an issue in Release build. I suppose it is only Debug build issue.
2. A bit slow switching between commits in a history ( fetching, I guess ).
3. Some commits cause crash. This is also debug-only issue and it exists in current master. I added this issue separately.
… 29 дек. 2019 г., в 01:16, Lucas Derraugh ***@***.***> написал(а):
What's the current status of this PR? Were all the apparent issues addressed?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@lucasderraugh |
|
Ready? |
|
@lucasderraugh Go-go-go! |
GitUp/Application/Document.m
Outdated
| @@ -338,7 +338,9 @@ - (void)windowControllerDidLoadNib:(NSWindowController*)windowController { | |||
| _searchResultsViewController.emptyLabel = NSLocalizedString(@"No Results", nil); | |||
| [_searchControllerView replaceWithView:_searchResultsViewController.view]; | |||
|
|
|||
| _quickViewController = [[GIQuickViewController alloc] initWithRepository:_repository]; | |||
| _quickViewModel = [[QuickViewModel new] configuredWithRepository:_repository]; | |||
There was a problem hiding this comment.
Make this an alloc init. I don't want new in this codebase lol. Make the initializer take the repository and don't have a separate config method.
GitUp/Application/Document.m
Outdated
| // static GICommitListViewController *theViewController = nil; | ||
| // if (theViewController != nil) { | ||
| // [_windowController.contentViewController dismissViewController:theViewController]; | ||
| // } | ||
| // GICommitListViewController *viewController = [[GICommitListViewController alloc] init]; | ||
| // viewController.results = commitsList; | ||
| // viewController.selectedCommit = commitsList.firstObject; | ||
| // theViewController = viewController; | ||
| // _searchResultsViewController.results = commitsList; | ||
| // _searchResultsViewController.selectedResult = commitsList.firstObject; | ||
| // | ||
| // if (_searchView.superview == nil) { | ||
| // [self _addSideView:_searchView withIdentifier:kSideViewIdentifier_Search completion:NULL]; | ||
| // } | ||
| // [_mainWindow makeFirstResponder:theViewController.preferredFirstResponder]; | ||
| // [_windowController.contentViewController presentViewControllerAsSheet:theViewController]; |
| @@ -16,7 +16,18 @@ | |||
| #import "GIViewController.h" | |||
|
|
|||
| @class GCHistoryCommit; | |||
| @class GCCommit; | |||
| @protocol GIQuickViewController__Delegate__Intentions | |||
There was a problem hiding this comment.
Don't use underscores. End with "Delegate" if that's what it is.
There was a problem hiding this comment.
GIQuickViewControllerIntentionsDelegate
or
GIQuickViewControllerDelegate
?
| @dynamic commit; | ||
| @dynamic delegate; | ||
| @dynamic list; |
There was a problem hiding this comment.
These aren't dynamic...you are implementing the setters but not the getters and they will crash upon access. Implement the getters if it's going to be a property. Otherwise implement them as methods with setters only. What would happen if you called the getter?
| if (paths.count == 0) { | ||
| return @[]; | ||
| } | ||
| // GCHistory *history = [self.repository loadHistoryUsingSorting:kGCHistorySorting_ReverseChronological error:error]; |
GitUpKit/Core/GCHistory.m
Outdated
| [commits addObject:newCommit]; | ||
| [newCommit release]; | ||
| commit = NULL; | ||
| // commit = NULL; // well, if we are here, we should terminate for-loop (count = git_commit_parentcount(commit)) |
There was a problem hiding this comment.
Don't leave commented out code. It should either be there with some action to take in the future or it shouldn't. There's no way to take further action on this.
GitUpKit/Core/GCHistory.m
Outdated
| // if (0 && (status == GIT_OK)) { | ||
| // printf("here: %s\n", git_commit_summary(commit)); | ||
| // git_commit_free(commit); | ||
| // continue; | ||
| // } |
GitUpKit/Core/GCHistory.h
Outdated
| - (instancetype)followRenames:(BOOL)followRenames; | ||
| - (instancetype)includeMerges:(BOOL)includeMerges; | ||
| - (instancetype)shouldIteroverWhenObjectNotFoundInTreeEntryByPath:(BOOL)shouldIteroverWhenObjectNotFoundInTreeEntryByPath; |
There was a problem hiding this comment.
These aren't initializers...you can't treat them as such. They need to follow init conventions if they're to initialize an object.
There was a problem hiding this comment.
@lucasderraugh
Do you really want all of them in one initializer? I remember the longest initializer ever in, well, 100 words?
There was a problem hiding this comment.
They don't need to be 1, but they need to follow standard Obj-C conventions of instance type instance methods that initialize objects need to start with init and they need to call super init. Otherwise you never initialized the object.
There was a problem hiding this comment.
+alloc return instancetype. If you invoke any property, say, [[UIViewController alloc] view], you will catch crash or even undefined behavior.
There was a problem hiding this comment.
I understand that. My point is that generally speaking in Obj-C we don't return instancetype unless we're creating a new instance. Here, you aren't doing that. You're mutating the existing instance and then returning it, which is very unexpected. I can't think of anything in Foundation or AppKit that would work this way.
There was a problem hiding this comment.
@lucasderraugh Yes, we could use struct with flags, it is more appropriate for ObjC since it doesn't have convenient alternatives ( no, NS_OPTIONS are not convenient ). But should we?
There was a problem hiding this comment.
@lucasderraugh Or we could add NSCopying and create new objects in these methods.
There was a problem hiding this comment.
Please delete these.
GitUpKit/Core/GCHistory.m
Outdated
| cleanup: | ||
| git_revwalk_free(walker); | ||
| free(fileName); | ||
| return [commits autorelease]; | ||
| } | ||
|
|
||
| - (NSArray*)lookupCommitsForFile:(NSString*)path followRenames:(BOOL)follow error:(NSError**)error { | ||
| return [self lookupCommitsForFile:path options:[[GCRepositoryHistoryFileOptions new] followRenames:follow] error:error]; |
There was a problem hiding this comment.
Again, alloc] initWith...]
| } | ||
|
|
||
| #pragma mark - Initialization | ||
| - (instancetype)initWithFollowRenames:(BOOL)followRenames includeMerges:(BOOL)includeMerges shouldIteroverWhenObjectNotFoundInTreeEntryByPath:(BOOL)shouldIteroverWhenObjectNotFoundInTreeEntryByPath { |
There was a problem hiding this comment.
Iterover isn't a word.
There was a problem hiding this comment.
shouldStopWhenObjectNotFoundInTreeEntryByPath
?
There was a problem hiding this comment.
Guess it's fine then.
|
@lucasderraugh Go-go-go? ( No, objective-c, but go! ) :) |
|
I did it! |
|
Just FYI, I still intend to get this PR into next release. Going to give feedback on your other PRs first. |
lolgear
changed the title
|
Will we get this PR in the next release as well? 😍 |
|
@lolgear I'm hitting a crash (abort statement) reliably when simply holding the down arrow key when I have the history view open. My repro commit is opening commit a502be4, then getting history on Document.m and start from the top item in the list selected and hold the down arrow. It's the same one I hit in my comment back here Sep 18, 2019 |
|
@lucasderraugh |
|
@lucasderraugh |
|
How are you reproducing the issue in master? |
Run debug build. |
|
I don't hit the same assert that I hit when running the history though. |
|
@lucasderraugh |
lolgear
force-pushed
the
quick_view_show_history_of_file
branch
from
a502be4 to
26ce6ff
Compare
…n added. kit: view controllers quick view contextual menu entry show file history has been added. application: document quick view delegate has been adopted. kit: quick view delegate protocol has been changed to match history commits. application: document delegate for quick view has been set. application: document show selected commits list in search view has been mocked up. kit: views quick view commits list has been added. application: document quick view with commits list have has been added. application: quick view model has been introduced. application: quick view model paging and moving have been fixed. application: document quick view model has been adopted. application: quick view model cleanup on reentering enter state. kit: quick view controller first attempt to show git history of file has been completed. application: document updating toolbar on reentering enter state of quick view has been added. application: quick view model selected commit setter has been added. project: quick view model has been added on application level. application: document toolbar sync in quick view has been fixed. kit: quick view controller delegate has been extended by did select commit method. application: quick view model guards against history updates suspended semaphore has been added. kit: views quick view animation has been added. kit: history lookup commits for file for loop null guard has been added. kit: views quick view cleanup. kit: components commit list view controller storage modifiers has been changed. kit: history lookup commits for file retain commit has been added. kit: views quick view cleanup. kit: quick view controller set commit equality guards have been added. kit: repository history file options have been added. kit: repository history file options have been updated. kit: repository history lookup commits for file parent and child trees have been swapped. kit: views quick view controller with commits list left view layout backward compatibility has been added. kit: cleanup. application: cleanup. kit: quick view controller possible fix for corrupted layout. kit: quick view set commit guards have been changed. kit: quick view hide file history variant has been added. document: quick view controller delegate protocol has been updated. kit: quick view layout constraints have been possibly fixed. kit: extensions nsview embedding has been added. kit: quick view layout has been changed to constraints. kit: quick view controller has been fixed. kit: xibs quick view controller has been fixed. kit: extensions view embedding has been cleaned up. kit: core gchistory file options have been cleaned up. kit: quick view controller with commits list has been extracted. kit: public headers have been updated.
lolgear
force-pushed
the
quick_view_show_history_of_file
branch
from
26ce6ff to
6d73e98
Compare
|
Very good, I finally rebased branch onto current master. |
|
@lucasderraugh |
|
Would love to get this in. 🙏 @lolgear have been maintaining this pull request since 2019. @lucasderraugh have you had a chance to look at this yet? 🙂 |
|
Just pushed 1.3.3 to the continuous release channel. Please verify if the assertion is still hit, as this is the reason it was never merged originally. If you believe the assert isn't hit anymore, then please let me know and I'll check the latest, but I don't have time to debug every commit change. |
|
I would absolutely love to have this merged 😍 |
|
|
|
Another year without this feature... |
|
As I said in my previous comment, if the assert is no longer hit then please let me know so I should go through and check it again. But the last changes we made I spend a solid week fixing the fallout and there is still some unresolved. I take seriously that I don't want to create regressions in this app as many have come to rely on it (including myself) and frankly it's not my job to make any PR that comes my way shippable. |
|
@lucasderraugh what do you think about making unit tests in order to now regress the app? |
|
We have unit tests, just didn't cover all the cases (and I resolved many issues to get the existing ones passing). When I resolve the new hit issues I intend to make tests around that. |
|
4 years and going... |
|
Did you fix the assert (I ask for the 3rd time) @lolgear |
Add SearchResultsCommitListView to QuickView.(Delayed)Add QuickView Window.(Delayed)assert(commit)Fixed
Addressed to
Issues
assert(commit)state.On old commits
Show file historyworks pretty well.On recent commits it fails at
assert(commit)for unknown reason.Discussions
Behavior of commits list.
Current left view on QuickView is
CommitListViewController. We should discuss a behavior of this controller.Should we hide it until person clicks
Show file history?Should we animate its appearance? ( it is hard to do in case of
AutoresizingMaskandNSLayoutConstraintfriendship ).Should we separate QuickView and HistoryOfFileView?
Should we add HistoryOfFileView on QuickView?
When should we remove HistoryOfFileView?