Native Commit window

[rdwampler]

This is a proposal for a new native commit window (See screenshot below.). It should not be merge in yet (only git is currently support and that is the bare minimum), but I wanted some initial feedback, regrading the overall approach. If not I will break out the responsibility of committing back to the bundles (like the current commit window). However, since TextMate already knows about some SCMs, extending it to handle committing might not be too much of a stretch. Also, I wanted to try and take advantage of the nice diffs in the git bundle hence the HTMLOutputWindow.

I do not know the best way to launch it so adding it to the view menu along with key mapping are really only for convience.

[sorbits]

I’ve long wanted the commit window to be native so I am really glad to see you take on this work.

I haven’t yet studied the actual patch but for approach I would prefer that the interface remains backwards compatible. Mainly so that we do not have to update the dozen bundles or so, that use the current commit window.

Presently the CommitWindow.app is in the SCM bundle which sets the TM_SCM_COMMIT_WINDOW variable to point to the executable, and all the SCM bundles use that variable to bring up the commit window.

This means that it should be possible to simply change the variable’s value to point to another executable that communicates with TextMate (to bring up the window) similar to how bundle commands can use $DIALOG to bring up “native” dialogs or $TM_MATE to open files.

I think ideally the commit window code would be moved to the dialog plug-in, although for this, the dialog plug-in should link with the OakTextView.framework to be able to use the proper text view, so for now, it’s OK to make it a part of TM itself, but I prefer to keep the commit window code as self-contained as possible.

That said, if the workflow is changed, there will of course be problems providing a backwards compatible commit window, but I think the ideal workflow is still to bring up a window with listing of files with check boxes, potential actions, and a field for the message. I.e. committing directly from file browser is not really possible because we still need to ask for a commit message, so having the file list in that window might seem a little redundant but not really a distraction.

As for embedding diff view’s in the SCM window: The current “API” for bringing up the commit window can take a list of commands that should be offered as actions per file, one of them is “diff”, so it seems to me that the current commit window would actually already be able to produce a diff (of checked files) based on the info it is already provided.

[sorbits]

I can btw help out with a skeleton for a replacement for TM_SCM_COMMIT_WINDOW (i.e. communication between this command and TextMate) — I would implement it similar to the mate command.

[rdwampler]

Thank you. I will work along these lines. Do you prefer I close this pull
request and reopen it later?

Yes, the current commit window does produce diffs, but this is less than
ideal since the window is always on top and afaik cannot be easily
dismissed. On smaller screens one needs to move the window to see the diff.

On Tue, Jan 28, 2014 at 10:23 AM, Allan Odgaard notifications@github.comwrote:

I can btw help out with a skeleton for a replacement for
TM_SCM_COMMIT_WINDOW (i.e. communication between this command and
TextMate) -- I would implement it similar to the mate command.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1210#issuecomment-33487931
.

[sorbits]

What I meant was just that the current commit window receive enough
information to actually produce a diff. I.e. in theory it should be
possible to make a replacement TM_COMMIT_WINDOW command that is
backwards compatible and provides diff (shown by default) without even
updating the bundles.

As for closing the pull request, I don’t mind if it’s kept open, it
might steer attention to your fork/work.

On 31 Jan 2014, at 4:10, Ronald Wampler wrote:

Thank you. I will work along these lines. Do you prefer I close this
pull
request and reopen it later?

Yes, the current commit window does produce diffs, but this is less
than
ideal since the window is always on top and afaik cannot be easily
dismissed. On smaller screens one needs to move the window to see the
diff.

On Tue, Jan 28, 2014 at 10:23 AM, Allan Odgaard
notifications@github.comwrote:

I can btw help out with a skeleton for a replacement for
TM_SCM_COMMIT_WINDOW (i.e. communication between this command and
TextMate) -- I would implement it similar to the mate command.

Reply to this email directly or view it on
GitHubhttps://github.com//pull/1210#issuecomment-33487931
.

---

Reply to this email directly or view it on GitHub:
#1210 (comment)

[rdwampler]

Please see the new commits. This version should be backward compatible with the current SCM bundles (only tested with git and hg repos). The UI is essentially the same as the current commit window, except we do not store previous commit messages and the --action-cmd buttons are now in a contextual menu.

I used some of the code from the current commit-window (copyrights are included in the appropriate files) so I am not sure how this will effect lincensing if pulled directly into TM tree.

[sorbits]

Thanks, I have been using this for a few days, here’s what I have noticed:

1. Table view has its bottom part hidden (incorrectly placed in the split view?). I wouldn’t mind dropping the xib and do the setup in code (with auto-layout). This will also make it easier to move the code to being native (instead of standalone app with MainMenu etc.)
2. The double-border between text view (status bar) and the table view should probably be suppressed. Unsure if this is from the table view or the split view. If the latter, I’m not sure we really need a split view, we could size the table view to number of items and cap at 10.
3. Untracked items selected in the file browser (when choosing “commit”) should default to being checked in the commit window, see textmate/commit-window@1da8a84.
4. Window key view order is arbitrary (should probably make the table view be the text view’s nextKeyView).
5. No action button (as an alternative to the context menu).
6. No history for previous commits (as you already mentioned).

And non-trivial stuff:

1. No shortcut for “commit” (⌅). Don’t really have a good idea here other than subclass OakTextView and watch for the key in keyDown:. Ideally though the key should only commit when not on a link (as enter is normally used to follow links).
2. Not running natively: This means slow startup, unable to cycle windows, and many functions unavailable (like most menu actions, clipboard history shared with TextMate, etc.) and settings from NSUserDefaults not read (e.g. font).

I can do the latter stuff, as I probably have a better idea of how to do that. I also don’t mind doing things from the first list, but since I consider this your project, I’ll await your input before I do anything.

As for copying code from commit-window, I’m not sure how much of the code we’ll end up importing, if a lot, it would be best to add it as a submodule and then work on the actual commit-window repository, as we would then get the full commit history (which show exactly who did what, and why, rather than the copyright comment headers that just show who created the file in Xcode).

Was there btw a reason for removing the inline table row buttons? I think having ‘diff’ as a one click action is nice (the “Modify” drop-down I’m less concerned about losing).

Btw: to use this version, I added the following to Default.tmProperties:

TM_SCM_COMMIT_WINDOW = "$CWD/CommitWindow.app/Contents/MacOS/CommitWindow"

[rdwampler]

Thanks. I would like to address the issues in the first list. The thought to do the setup in code did crossed my mind. If you can build out the skeleton to run this natively that would be great :).

As for the inline table row buttons, I initially wanted to limit the amount of code copied from commit-window as much as possible. However, if in the end we decide to move this out to a submodule and work on commit-window, then I feel comfortable putting them back. But I would prefer to keep the "Modify" actions in the contextual menu.

btw I did not realize that document stores backups of the ``document_ptr` contents so there are probably a lot of orphaned documents sitting under the Application Support folder. I explicitly close it now when canceling/committing to ensure it gets removed. I will push this change up later along with the other changes mentioned above.

[sorbits]

I have pushed commit 0c4f085 to the native-commit-window branch which introduces a new TM_COMMIT command that opens a skeleton commit window in TextMate and returns a result.

I also pushed a few commits to a commit-window branch. These are minor changes/improvements for your pull request.

As for No shortcut for “commit” (⌅) above, having used the OakTextView based commit window for a while now, it seems the most natural approach would be to allow ⌘S and ⌘W in the commit window, so basically have it mimic a regular editor window.

I used TM_COMMIT for the new command, it should of course be TM_SCM_COMMIT_WINDOW once it is argument compatible with the current command.

[rdwampler]

Thanks, I have been working on merging these changes into the native-commit-window branch.
Although, I am running into one issue trying to launch mate within OakCommitWIndow. The command hangs indefinitely. Other commands, e.g., git diff ..., run successfully (after setting the environment and cwd). However, I can get it to work if I send the request back to OakCommitWindowClient and run mate there. I am missing something?

btw, I noticed that NSPanel is actually responding to the action message cancel:. Not a big deal, but I thought I would bring it to your attention, since a similar pattern is used by OakRunCommandWindowController, etc.

[sorbits]

If you are calling ‘mate’ from TextMate itself, you must do this on
a background thread, since it’s TextMate’s main thread that responds
to ‘mate’.

I think you can do something like this:

dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 

0), ^{
io::exec(getenv("TM_MATE), …, NULL);
});

As for NSPanel responding to cancel:, I think I normally have cancel:
just close the window, and expect the potential NSPanel implementation
to do the same, so there should be no difference in who responds to the
method.

On 17 Mar 2014, at 22:21, Ronald Wampler wrote:

Thanks, I have been working on merging these changes into the
native-commit-window branch.
Although, I am running into one issue trying to launch mate
within OakCommitWIndow. The command hangs indefinitely. Other
commands, e.g., git diff ..., run successfully (after setting
the environment and cwd). However, I can get it to work if I send the
request back to OakCommitWindowClient and run mate there.
I am missing something?

btw, I noticed that NSPanel is actually responding to the action
message cancel:. Not a big deal, but I thought I would bring it
to your attention, since a similar pattern is used by
OakRunCommandWindowController, etc.

---

Reply to this email directly or view it on GitHub:
#1210 (comment)

[rdwampler]

Thanks! The above worked.

[rdwampler]

Please see the new commits. Details are in the commit messages. Any feedback is appreciated as always.

[sorbits]

I had an issue with an exception (related to distributed objects) which so far seems to be fixed by 021367f.

We may need to change from NSPanel → NSWindow since we have other panels that should interact with “active text field”, for example the find dialog or clipboard history. The current window style also looks a little bit non-standard — I think we could extend the top gradient below the “Previous Commit Messages” (so the look is similar to e.g. ⌘T). This is done with setContentBorderThickness:forEdge: (and I think it requires also using setAutorecalculatesContentBorderThickness:forEdge:).

With respect to calling mate for showing file diffs, one can use --name to set a custom display name (though I’m not sure what a good display name would be). Coincidentally I recently added path::escape as a shared shell_quote: c73ad25.

Otherwise no comments — let me know if you have any plans, otherwise I’ll review the code and merge it (after having used it a bit to ensure there are no regressions).

It’s btw worth mentioning that TextMate does not show the HTML output window for commands with HTML output before the command writes something to stdout. This means the commit commands could delay the output window until after the commit window has been closed — though I had a quick look at Git → Commit… and it seems to be a larger project to make it act this way (might be easier to just recreate the commit command).

[rdwampler]

Thanks, looking through my logs I had a similar exception.

I pushed a new series up, the main changes were to incorporate your suggestions concerning the window style and using path::escape. The latter helped to simplify most of code for handling paths, especially in absolutePathForPath.

Besides, maybe, the diff name (perhaps a/display_name(path) - b/display_name(path)?), I do not have any plans for further enhancements. For the future, I thought it could be nice to potentially filter the previous commit messages by TM_PROJECT_DIRECTORY (i.e., repo). In the meantime, I will look into the commit commands.

[sorbits]

I pushed a few changes to native-commit-window branch.

I don’t see anything blocking this from going into master, so planning to merge later. Unless you have objections, I will likely squash some of the commits.

[sorbits]

The previous commit window also adds to history if user cancels the commit. I find this useful as writing a commit message sometimes makes you realize that further changes are needed, and thus you cancel but would like to re-use the commit message.

Btw: I assume you looked to other sources for my coding style, I really appreciate this effort! The only thing you missed is that I use camelCase for local variables.

[rdwampler]

I look over the changes you made and appreciate the improvements. However, I ran into an issue with minimizing the commit window when Minimize windows into application icon is enabled. The window disappears after un-minimizing, but it is still in focus. Not sure if this is due to us using NSFloatingWindowLevel with NSMiniaturizableWindowMask in a non-standard way. I am running 10.9.2.

I don't have any objections, so please feel free to squash the commits as needed. You can drop 93c5658 to go back to the original behavior of ignoring NSPanels (maybe keep the id).

[sorbits]

Merged as d4882de...9018e8b — thanks a lot for your contributions!