Gitx GitX → 0.8

Comments and changes to this ticket

• 

  Dave Grijalva January 26th, 2009 @ 09:58 PM

  In Leopard, I believe there's a way to have the application register to be notified of updates within a directory. This could be used to trigger UI updates automatically based on changes to the filesystem.
  

  It should be pretty simple to have this spawn an NSOperation that would check to see if the UI needs updating. The tricky part will be not wasting too much time on file changes that do not effect the presentation of the history view.

• 

  elliottcable January 26th, 2009 @ 10:03 PM

  Yes yes yes please please please with a cherry on top and possibly unlimited lifetimes' supplies of chocolate syrup?

  searches Unicode for a pleading face, fails to find one

• 

  Dave Grijalva January 26th, 2009 @ 10:22 PM

  The library for handling this is called FSEvents. There is a "File System Events Programming Guide" available through the xcode documentation browser.

• 

  Dave Grijalva January 26th, 2009 @ 11:28 PM

  Just added a patch to do this:

  http://github.com/dgrijalva/gitx...

• 

  Dave Grijalva January 26th, 2009 @ 11:45 PM

  Oops. Missed some files. Patch includes previous commit and http://github.com/dgrijalva/gitx...

• 

  Pieter de Bie January 27th, 2009 @ 09:42 PM

  • Milestone set to “0.7”
  • State changed from “new” to “open”

  Hey,

  I'll take a look at it later this week. From a quick glance, it looks like a very clean implementation

• 

  Dave Grijalva January 29th, 2009 @ 12:21 AM

  Added a new commit: 549a0b39795d129564fdb19959b7f2913db4f0af to address issues

• 

  Dave Grijalva March 23rd, 2009 @ 02:51 AM

  I refactored the history watcher to be able to watch the history, index, and working directory. Now both the history and commit views update automatically. It also looks a little deeper into the change notifications being sent to make sure it only updates the correct views.

  Commit: dbe5fb7a6d021f8dda9878216cfa455022de5d28

• 

  Pieter de Bie March 24th, 2009 @ 01:59 AM

  Hey,

  You might have noticed I've done a bit of cleaning-up in the bug tracker but haven't responded to this yet.

  I think it is a very cool feature to have, but it definitely needs to be user-enabled. I can add that if you want.

  However, it is currently a bit messy and I'm not sure how to review this properly. I think a lot has changed. Can you perhaps rebase it on top of my master in one or a few clean commits? That would make it a lot easier for me to review.

• 

  Johannes Gilger June 9th, 2009 @ 10:48 PM

  Hi, I'd second the request for rebasing this. If you're afraid of having to constantly rebase and therefore merge this have a look at "git rerere" which should take some load of your shoulders.

  This would really be one of the killer-features for 0.7!

• 

  Dave Grijalva June 9th, 2009 @ 11:00 PM

  I'm crunching to get something released at work this week. I'll clean this up asap.

• 

  Pieter de Bie June 11th, 2009 @ 04:04 PM

  I've cleaned this up a bit here:

  http://github.com/pieter/gitx/tree/dg/fsevents

  What I've done mostly is remove the Bookmark support that was still in your branch, and put it all in one commit, and rebased that on top of my latest master.

  I really like the idea, and I think this last version is a lot better than the earlier ones. It's still a large patch though, so it would be nice if you could split it up in the three parts I mentioned in the commit message. After that's done, we can review them one by one and see what needs to change. For one thing, I'd like all the classes to be in separate files. We also add private methods by creating an anonymous category within the implementation file. Then there's some style differences, like how methods are declared (- (void)doSomething:(BOOL)it; is our style). I haven't looked at the patch in detail yet, but I'll try to find the time for that after the split.

  One thing I'd like to see is support of per-file change notice, in particular for the .git/refs, .git/packed-refs and .git/HEAD files. This can be done by adding a new class which receives the notifications, and then reads the directories and checks for file changes.

  Another thing is the use of notifications. We've avoided them so far in GitX. It'd be nice if we could use a delegate pattern instead, but if that's too much work, I don't really mind.

  You also have to take care with FSEvents that if changes in subdirectories happen quickly after eachother, these might be aggregated into a single event, in which case you don't only have to look at file changes in the top directory, but also in any subdirectories.

  Finally, we can't really call md5 everytime the event fires. I think we should just look at the last modification time. That should be accurate enough.

  I'd really like to see this in 0.7 too :)

• 

  Pieter de Bie June 11th, 2009 @ 04:48 PM

  Two other things:

  1) I don't think the watcher should be part of the windowcontroller,
  but rather the PBGitRepository itself.

  2) You can extract most of the code in the two viewcontrollors to
  their common superclass

• 

  Pieter de Bie July 15th, 2009 @ 07:58 PM

  Hey Dave, any progress on this?

• 

  Dave Grijalva July 22nd, 2009 @ 06:49 AM

  I don't think per-file notifications are required for the feature I've been working on. It might be nice to have, but all we really need to know is that something has changed and ask the view to refresh.

  The same goes for your note on aggregate change events. It's true that, to get per file change information, you have to care about that, but all we need to care about is that something has changed.

  As for notifications vs. delegation, the precedent set by Apple is that delegation is for when one object needs to watch something and notifications are for when there are multiple interested parties. Notifications are low cost and, as long as you're careful about which thread you fire them on, exactly the right choice for this. Actually, using notifications makes it easier to move the watcher into the repository and out of the individual controllers.

• 

  Dave Grijalva July 22nd, 2009 @ 06:52 AM

  I'm really not sure what you want me to do as far as rebasing and re-chopping the commits. How about I get the feature working and then we can jump on iChat at some point and you can describe exactly what you want?

• 

  Pieter de Bie July 22nd, 2009 @ 11:54 AM

  sounds like a plan!

• 

  Johannes Gilger September 1st, 2009 @ 09:17 AM

  Is there still any hope for this for 0.7? I haven't reviewed any code, so I'm asking purely for informational purposes.

  I saw that a few days ago Pieter twittered about a GitX nightly which might become 0.7, so I'm guessing he doesn't expect this to be in there.

• 

  Dave Grijalva September 2nd, 2009 @ 08:04 AM

  I have a branch in my fork that's almost done. I've been super busy at work.

• 

  Johannes Gilger September 2nd, 2009 @ 06:28 PM

  I've looked at your changes and they look straightforward and the notifier-stuff seems good to go. However the latest commit doesn't work since you've put the initializer at the wrong spot. You've put it into the initWithURL method while it has to go in the readFromURL-method.

  Also there is a small error in PBGitRepositoryWatcher.m:
  [[NSNotificationCenter defaultCenter] postNotificationName:PBGitRepositoryEventNotification object:self userInfo:eventInfo];

  Where it should be object:repository.

  If you've no objections I'd like to try the following things these evening:
  - Squash your patches (and my small bugfix) and split them into two again (retaining your authorship). - Introduce a small preference-switch for enabling/disabling the watcher.

  Anything else that's on your TODO-list or that isn't working? The refreshing seems to work fine for me.

• 

  Johannes Gilger September 3rd, 2009 @ 08:15 PM

  OK, I think this feature is ready to go. I've squashed and re-split your patches into three commits (each of which still compiles) with descriptive commit-messages and added a fourth commit introducing the preference for the watcher myself.

  With my patch you have to re-open a repository when you change the preference, but I think this is just fine for the start. Later one could think of making the pref "live" and maybe even giving some visual indication that the feature is enabled. It's enabled by default now by the way.

  I've rebased it on my bugfixes, which also contains a patch with two other preferences so in order to easily take these changes Pieter would have to get the one on my bugfixes-branch as well (sorry for making that decision for you). Unless Dave has any objections I'd say we take it as it is right now and wait for further feature-requests when 0.7 is released.

  I've attached a screenshot of how the preferences look now.
  

• 

  Dave Grijalva September 3rd, 2009 @ 08:27 PM

  Should be fine. The only thing I've noticed is that rapid fire reloads of the commit view can cause some weirdness, but that was in an older build. Seems like asking it to reload while it's still reloading is causing issues. If it's still happening, the best fix would probably be to make the reloading code safer. Maybe use a common operation queue so reload requests can't stomp on each other.

  I would say give it a spin and if everything seems go, ship it.

• 

  Johannes Gilger September 3rd, 2009 @ 08:40 PM

  OK, i've spotted a small error in my code and ammended the last commit (i.e. mine).

  The problem is that PBGitRepository uses both initWithURL and readFromURL depending on the way gitx is started.

  Anyway, the check for the PBGitDefault value is now done directly in the watcher, I hope this doesn't go too much against any design patterns (I'm still learning how to structure this stuff).

  BTW, are you on the ML Dave? Just curious.

• 

  Johannes Gilger September 3rd, 2009 @ 08:41 PM

  And of course my branch at GitHub is not bugfixes but dg/fsevents, which forks from my bugfixes.

• 

  Dave Grijalva September 3rd, 2009 @ 08:54 PM

  There's a mailing list?

• 

  Johannes Gilger September 3rd, 2009 @ 11:54 PM

  Yep, you can have a look at it (and subscribe) at http://groups.google.com/group/gitx/

  If you're planning on further contributing to GitX it would be best if you subscribed, it's still fairly low-traffic.

• 

  Johannes Gilger September 8th, 2009 @ 12:21 PM

  • Milestone changed from “0.7” to “0.8”

  Hi Dave,

  sorry this cool feature didn't make it into 0.7. Pieter had some reservations about it interfering with simultaneous git-operations on the index and about performance in commit-view (both problems that don't result from the FSEvents but rather from the way we reload stuff).

  However your commits are split now in the dg/fsevents-branch and I guess you don't have to do that much more. Pieter will probably include the patches (limited to history-view for the beginning) into master not too far from now, and then we can safely develop anything else on top of that.

  Thanks for working on this anyway!

• 

  Johannes Gilger September 10th, 2009 @ 12:13 PM

  Hm, we should think about what commands get issued as a result of refreshing either view (what git-commands) and whether these can create race-conditions or deadlocks.

  I'm not that familiar with the index, but the most "problematic" call in PBGitIndexController is to update-index, right? When should that command not be called? I'd really like to make this thing work, but since it has worked for me I need to know the problems that could/do occur ;)

• 

  Jim Zajkowski October 26th, 2009 @ 07:56 PM

  At the very least can GitX just refresh when it's reactivated, the way TextMate re-reads its directory structure?

• 

  Dave Grijalva March 13th, 2010 @ 02:27 PM

  • State changed from “open” to “feature”

  [bulk edit]