Stage menu #30

ssp · 2017-02-07T01:27:19Z

As discussed in #24 and #25, this commit creates a »Stage« menu sets up the contextual menu in the xib file instead of in code and makes the action handling of the contextual menu reusable for the main menu bar.

As the general structure of the involved views/controllers still seems to be a little confusing to me, feel free to advise me of how to improve this (unless doing so would involve refactoring the whole thing ;).

tiennou · 2017-02-16T00:40:20Z

Classes/Controllers/PBGitCommitController.m

+	if (!(userName && userEmail)) {
+		return [[repository windowController]
+				showMessageSheet:NSLocalizedString(@"User‘s name not set",
+												   @"Title for sheet that the user’s name is not set in the git configuration")


Broken indentation here

@tiennou – looks OK on my machine, I suspect a tabs vs spaces issue here.

As far as I can tell, the project uses both of those indentation styles, so it’s hard to tell what’s right and what is wrong.

My Xcode seems to default on using tabs on the project.

Just FYI you can see the diff ignoring whitespace by appending ?w=1 to the diff url:

e.g. https://github.com/gitx/gitx/pull/30/files?w=1

And the tabstop width can be set with ?ts=4.

Xcode almost does the nice method of tabs for indentation, spaces for alignment. Instead it does tabs for indentation, both tabs and spaces for alignment...

(vim with list/listchars)

tiennou · 2017-02-16T00:40:47Z

Classes/Controllers/PBGitCommitController.m

+				showMessageSheet:NSLocalizedString(@"User‘s name not set",
+												   @"Title for sheet that the user’s name is not set in the git configuration")
+				infoText:NSLocalizedString(@"Signing off a commit requires setting user.name and user.email in your git config",
+										   @"Information text for sheet that the user’s name is not set in the git configuration")];


nitpick: indent

tiennou · 2017-02-16T00:40:53Z

Classes/Controllers/PBGitCommitController.m

+	}
+
+	NSString *SOBline = [NSString stringWithFormat:NSLocalizedString(@"Signed-off-by: %@ <%@>",
+																	 @"Signed off message format. Most likely this should not be localised."),


nitpick: indent

tiennou · 2017-02-16T00:42:12Z

Classes/Controllers/PBGitCommitController.m

 		return;
 	}

 	if ([[cachedFilesController arrangedObjects] count] == 0) {
-		[[repository windowController] showMessageSheet:@"No changes to commit" infoText:@"You must first stage some changes before committing"];
+		[repository.windowController


Ouch. Maybe use local NSStrings so it's clearer ?

Giving that a shot – although I’m not entirely convinced that it really improves things (basically trading long lines against additional lines).

Adjacent string literals get concatenated, if that helps:

NSLocalizedString(@"You need to stage some changed files before committing " "by moving them to the list of Staged Changes.", @"Information text for sheet that you need to " "stage changes before creating a commit")

The @ is optional on subsequent NSString literals.

tiennou · 2017-02-16T00:42:26Z

Classes/Controllers/PBGitCommitController.m

-	if ([commitMessage length] < 3) {
-		[[repository windowController] showMessageSheet:@"Commit message missing" infoText:@"Please enter a commit message before committing"];
+	if (commitMessage.length < kMinimalCommitMessageLength) {
+		[repository.windowController


tiennou · 2017-02-16T00:42:53Z

Classes/Controllers/PBGitCommitController.m

+	NSString *reason = notification.userInfo[kNotificationDictionaryDescriptionKey];
+	self.status = [NSString stringWithFormat:
+				   NSLocalizedString(@"Commit failed: %@",
+									 @"Message in status bar when creating a commit has failed, including the reason for the failure"),


nitpick indent

tiennou · 2017-02-16T00:46:29Z

Classes/git/PBGitIndex.m

@@ -44,6 +44,7 @@ - (NSArray *)linesFromData:(NSData *)data;
 @interface PBGitIndex () {
 	dispatch_queue_t _indexRefreshQueue;
 	dispatch_group_t _indexRefreshGroup;
+	BOOL amend;


Keep the property, but name the ivar _amend. That'll be less surprising (properties by default use a underscored ivar if there's one).

tiennou · 2017-02-16T00:47:46Z

English.lproj/MainMenu.xib

                                </connections>
                            </menuItem>
-                            <menuItem isSeparatorItem="YES" id="DoS-f9-CnX"/>
-                            <menuItem title="Open in Terminal" keyEquivalent="T" id="942">
+                            <menuItem title="Amend Commits" keyEquivalent="a" id="W5p-JV-xeS">


"Amend Commit"

tiennou · 2017-02-16T00:55:51Z

Classes/Controllers/PBGitCommitController.m

+}
+
+
+- (PBGitIndex *) index {


Any reason you're keeping it around ? I think it could go away...

It is used from PBGitIndexController and PBWebChangesController, so those would need reworking to not repeat that chain of accessors over and over.

I didn’t want to get into that now, because it’s not really clear to me what the relations between those controllers and the objects they manipulate should be.

tiennou · 2017-02-16T00:56:43Z

Classes/Controllers/PBGitHistoryController.m

@@ -590,19 +590,20 @@ - (void)showCommitsFromTree:(id)sender
 - (void) checkoutFiles:(id)sender
 {
 	NSMutableArray *files = [NSMutableArray array];
-	for (NSString *filePath in [sender representedObject])
+	for (NSString *filePath in [sender representedObject]) {


I guess you want a line break there

If we want to go down such a road, we should probably write down on the desired locations for spaces first and try to get the code into that shape. Is that worth the effort?

Would using clang-format in some way be helpful with this?

Sorry, it was late, and the churn was getting hard to process.

I know the current style is messy (is there any code which isn't), I don't have an opinion on what to do about that yet. It's just distracting to read unrelated whitespace-bracing changes while you look at +667/-447 lines.

👍 for clang-format. If you have the will to make it work 😉. I'm mostly in-line with the GitHub Objective-C Style Guide.

tiennou · 2017-02-16T00:56:50Z

Classes/Controllers/PBGitCommitController.m

@@ -177,7 +175,7 @@ - (void) commitWithVerification:(BOOL) doVerify

 	NSString *commitMessage = [commitMessageView string];
 	if ([commitMessage length] < 3) {
-		[[repository windowController] showMessageSheet:@"Commitmessage missing" infoText:@"Please enter a commit message before committing"];
+		[[repository windowController] showMessageSheet:@"Commit message missing" infoText:@"Please enter a commit message before committing"];


tiennou · 2017-02-16T00:58:25Z

Classes/Controllers/PBGitCommitController.h

@@ -17,10 +17,6 @@
 @class PBCommitMessageView;

 @interface PBGitCommitController : PBViewController {
-	// This might have to transfer over to the PBGitRepository
-	// object sometime
-	PBGitIndex *index;


churn: can you split that change ?

tiennou · 2017-02-16T01:01:17Z

Classes/Controllers/PBWebChangesController.m

@@ -41,7 +41,7 @@ - (void)closeView

 - (void) didLoad
 {
-	[[self script] setValue:controller.index forKey:@"Index"];
+	[[self script] setValue:controller.index  forKey:@"Index"];


tiennou · 2017-02-16T01:04:04Z

Classes/Util/PBOpenFiles.h

 @interface PBOpenFiles : NSObject

-+ (void)showInFinderAction:(id)sender with:(NSURL *)workingDirectoryURL;
-+ (void)openFilesAction:(id)sender with:(NSURL *)workingDirectoryURL;
+ (void)openFiles:(NSArray<PBChangedFile *> *)selectedFiles with:(NSURL *)workingDirectoryURL;


That code is supposed to work with NSString or PBChangedFile and whatever you can throw at it (hence -selectedURLsFromSender:...`, so I'd prefer to keep it that way 😉

My line of thinking was the other way round … a) the code does only work for PBChangedFiles and b) it looks like a Utility to handle opening files.

I can see how it makes sense to add the ability to open other objects here when it’s needed. However, seeing this as a utility for opening files, I don‘t think it should receive UI-Elements as an input and expect them to represent specific objects. My idea was to extract the data from the UI at the controller layer and let this class handle opening the files.

That code was originally in PBGitRepositoryDocument, where the responder chain would get to it in some way. And some things can throw paths at it (like, right-click-opening a changed file in the history view commit pane), which is why I wrote it that way. Now that we have submodules, we have that (generic) method, the stuff you've added for submodules, and a bug because right-click-open in the history view doesn't do the same thing...

tiennou · 2017-02-16T01:06:45Z

Classes/Views/PBFileChangesTableView.m

@@ -15,11 +15,11 @@ @implementation PBFileChangesTableView

 - (NSMenu *)menuForEvent:(NSEvent *)theEvent
 {
-	if ([self delegate]) {
+	if(self.indexController) {


indent: missing space after if-statement. Also, I'd consider that churn 😉.

Maybe redeclare -(id)delegate as -(PBGitIndexController *)delegate in the header if that's what you're after ?

nice idea!
… turns out that alone is not sufficient, I seem to need an implementation to match the header as well.

tiennou · 2017-02-16T01:14:08Z

Classes/Controllers/PBGitIndexController.m

-		ignoreItem.target = self;
-		[menu addItem:ignoreItem];
-	}
+ (NSString *) getNameOfFirstFile:(NSArray<PBChangedFile *> *) selectedFiles {


Can you move those class methods in PBTableViewWhatWasItsName instead of keeping them here ?

tiennou · 2017-02-16T01:18:22Z

I'm starting to love that view hierarchy 😉. Normally you could move the gist of -[PBFileChangeTableView validateMenuItem:] inside PBGitIndexController and it would just work...

This should also include the solution suggested in rowanj#24. Moves all menu activation/hiding to the validation method, so it can be used from the main menu as well as the contextual menu.

…ments

They are available in the main menu now.

tiennou · 2017-02-20T14:22:14Z

I'm sorry, I'm NAK-ing that. I can't get myself to accept the weirdness that's going on in PBFileChangesTableView, with all the proxing stuff from other controllers down to a view thingy.

On the other hand, I tried it for a few days, and it seemed to work quite well (that menu is nice !), but as I have stuff cooking w.r.t to clarifying the view hierarchy which this PR step all over, let's do this : I'll propose the changes I want to make, then the massive redo-rebase of this one that will result is on me. Does that sound fair ?

I really want to cleanup some of that stuff before the PR starting coming in 😓

ssp · 2017-02-23T00:41:34Z

@tiennou no problem, that’ll work for me.

Just let me know what else you need me to do.

ssp · 2017-02-26T22:01:02Z

Closing this PR, now that @tiennou ’s reworked version has been merged with #50 / #55.

ssp force-pushed the stage-menu branch 3 times, most recently from 5a3ff52 to 9e38c57 Compare February 9, 2017 22:29

ssp mentioned this pull request Feb 16, 2017

Various repository cleanups #40

Merged

tiennou requested changes Feb 16, 2017

View reviewed changes

ssp added 6 commits February 16, 2017 23:09

Fix Source file order in project file

d77e74f

Introduce Stage menu rowanj#25

4a57ea6

This should also include the solution suggested in rowanj#24. Moves all menu activation/hiding to the validation method, so it can be used from the main menu as well as the contextual menu.

don’t @synthesize PBGitCommitController.index

4da2f22

PBGitCommitController: NSLocalizedStrings + minor consistency improve…

92ec6d3

…ments

Remove flashing Stage buttons beneath commit message

6155c37

They are available in the main menu now.

Add stage menu item to open submodule in GitX rowanj#29

0093c9b

ssp force-pushed the stage-menu branch from 9e38c57 to 0093c9b Compare February 17, 2017 00:11

elia mentioned this pull request Feb 21, 2017

Set ⇧⌘A as key equivalent for "Amend" #24

Closed

tiennou mentioned this pull request Feb 25, 2017

Stage menu redux #55

Merged

ssp closed this Feb 26, 2017

Stage menu #30

Stage menu #30

Uh oh!

Conversation

ssp commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssp Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssp Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Feb 16, 2017

Uh oh!

tiennou commented Feb 20, 2017

Uh oh!

ssp commented Feb 23, 2017

Uh oh!

ssp commented Feb 26, 2017

Uh oh!

Uh oh!

ssp commented Feb 7, 2017 •

edited

Loading

ssp Feb 16, 2017 •

edited

Loading

ssp Feb 16, 2017 •

edited

Loading