work begins

[Steve Borho]

Granted, this is just a one-line change, but it proves out a lot of things I was concerned about.  For one, the diff lexer is definitely emitting fold sections for each diff chunk (this was a lucky guess on my part).  For two, the fold margin can be added to the commit diff view without any side-effects to the other views.

For convenience, we could easily enable folding for the file/annotate views as well, for all of the lexers which support folding.  But we might want to use a different folding style so that the features are not confused.

--
Steve Borho

[Steve Borho]

<picture snipped>

Granted, this is just a one-line change, but it proves out a lot of things I was concerned about.  For one, the diff lexer is definitely emitting fold sections for each diff chunk (this was a lucky guess on my part).  For two, the fold margin can be added to the commit diff view without any side-effects to the other views.

For convenience, we could easily enable folding for the file/annotate views as well, for all of the lexers which support folding.  But we might want to use a different folding style so that the features are not confused.

 

And I've run into the first roadblock.  QSci doesn't have a signal to tell you when a fold is collapsed or expanded, and it does not emit marginClicked signals for the fold margin.  So I either have to poll the list of collapsed folds, or I'll have to do my own faked folding margin (which may have been a requirement in the end anyway, for when hiding of excluded changes is disabled).

Current patch is attached, for the curious

--
Steve Borho

[Yuya Nishihara]

On Sat, 22 Dec 2012 22:40:56 -0600, Steve Borho wrote:
> And I've run into the first roadblock. QSci doesn't have a signal to tell
> you when a fold is collapsed or expanded, and it does not emit
> marginClicked signals for the fold margin.

About marginClicked, I guess the low-level SCN_MARGINCLICK signal can be
used instead. But it may be emitted before changing fold state.

Qt4/qsciscintilla.cpp:

connect(this,SIGNAL(SCN_MARGINCLICK(int,int,int)),
SLOT(handleMarginClick(int,int,int)));

void QsciScintilla::handleMarginClick(int pos, int modifiers, int margin)
{
...
if (fold && margin == foldmargin)
foldClick(line, state);
else
emit marginClicked(margin, line, Qt::KeyboardModifiers(state));
}

Regards,

[Steve Borho]

Nice idea, this mostly works:

    self.sci.SCN_MARGINCLICK.connect(self.marginClicked)

    def marginClicked(self, pos, modifiers, margin):

        # raw margin clicked event, received before folding has responded

        assert margin == 3

        line, index = self.sci.lineIndexFromPosition(pos)

        assert index == 0

        print line, modifiers

It would have to queue a single-fire timer to query the state of the collapsed folds and update its internal state accordingly.

But I've just about convinced myself that I need to avoid the built-in folding feature anyway, in order for the 'show excluded changes' toggle to eventually work.  I'm now re-considering just how closely we want the change selection feature to look like folding, or whether it should appear more unique.

--
Steve Borho

[Steve Borho]

Hello,

Attached is a new patch.

Change selection looks to be essentially working.  The file list checkboxes are being updated correctly, and toggling those checkboxes manually also updates the chunk selection state appropriately.  Refresh seems to work correctly.  I would appreciate testing with non-ascii filenames; I suspect I mixed unicode in some places that don't expect it.

Code that is still missing;

* code which recognizes partial commit and uses the memctx calls (honestly, doesn't look like much work)

* adding 'all/none' buttons to fileview toolbar, when folding is enabled

* adding '(%d of %d changes excluded)' string to fileview toolbar, when folding is enabled

* supporting 'show excluded chunks' toggle button.  This will require a lot of work, but all within a few existing fileview functions: enableDiffFolding(), _showFoldMargin(), marginClicked() and updateChunk() 

Request for Opinions:

The 'change has been excluded from commit' annotations are tedious after you've seen a few of them.  Would it be more appropriate to have a 'folded changes are excluded from commit' annotation, and to only annotate the first folded chunk in each file?

My plan is to add the remaining items in the order they are listed above, and to begin publishing nightly builds once it is roughly functional.

--
Steve Borho

[Steve Borho]

I moved some code around to place the new features where it made the most sense, and then I checked in and pushed what I have now. Change selection works pretty closely to how I think it will work 'out of the box' but the commit tool currently ignores the partial selections.

I hope to have the commit portion working tomorrow, as I'm running out of vacation time.

--
Steve Borho

[Angel Ezquerra]

I just tested this. It looks pretty good! I think _a lot_ of people
will be very, very happy when they discover this new functionality on
the next version.

That being said, there are a couple of things that I'd like to
comment, some of which I already mentioned in the past:

1. I find the fact that this uses regular fold icons confusing. I
think this may lead people to not commit some changes by mistake.
Maybe a "checkbox" style icon would be better.
2. I think the "unchecked" (I stubbornly refuse to call them "folded"
:-) ) changes should be colored differently (perhaps in light gray).
3. I really do not like the fact that unchecking a file on the file
list folds all changes on that file. It is perhaps consistent, but I
don't think it is very convenient.
4. I am not super happy with the "folded changes are excluded from
commit" message. While it is nice that it appears right on the first
change that you uncheck, the fact that it gets mixed with actual file
contents is not that nice. What about showing a "tooltip" or an
overlay with that message when you uncheck the first chunk? The
tooltip could disappear after a few seconds or if the user clicks
anywhere... Also, I think the message should say "unchecked" rather
than "folded" if we are to have a mode where there is no folding.

Personally I am not super thrilled with the folding. I would find it
ok as an option, but not as the default behavior. I think it has good
properties but overall the bad outweights the good IMHO.

A couple extra comments:
- I'd like to get a clearer picture of the actual implementation
details. I did not go through the revisions on a lot of detail, but
getting an overview of the design (the partialcommit "extension", etc)
would be nice. In particular I'd like to better understand the memctx.
Is this something that was developed a while ago for mercurial core?
- What are the limitations of this? It seems this cannot work with
amend, can it? What about mq? These would be important limitations
which would be great to overcome if possible.

Despite all this comments I must say that I am very thrilled by this
and a bit impressed by the fact that this is already so functional.

Cheers,

Angel

[Steve Borho]

I'll summarize that as -1 for folding.

 

2. I think the "unchecked" (I stubbornly refuse to call them "folded"
:-) ) changes should be colored differently (perhaps in light gray).

and -1 for folding

 

3. I really do not like the fact that unchecking a file on the file
list folds all changes on that file. It is perhaps consistent, but I
don't think it is very convenient.

This behavior is quite consistent with how hgtk change selection worked.  All the changes were shown in the unselected state when the file was unchecked.  The only difference is the folding, so .. -1 folding.

 

4. I am not super happy with the "folded changes are excluded from
commit" message. While it is nice that it appears right on the first
change that you uncheck, the fact that it gets mixed with actual file
contents is not that nice. What about showing a "tooltip" or an
overlay with that message when you uncheck the first chunk? The
tooltip could disappear after a few seconds or if the user clicks
anywhere... Also, I think the message should say "unchecked" rather
than "folded" if we are to have a mode where there is no folding.

The annotations are, to be precise, 3 lines of code that I added as an experiment to see how they looked.  So far I'm not impressed with them either but was going to reserve judgement until getting more opinions.  Removing them will take all of about 15 seconds, most of that will be spent writing the commit summary.

 

Personally I am not super thrilled with the folding. I would find it
ok as an option, but not as the default behavior. I think it has good
properties but overall the bad outweights the good IMHO.

ok, -1 folding

 

A couple extra comments:
- I'd like to get a clearer picture of the actual implementation
details. I did not go through the revisions on a lot of detail, but
getting an overview of the design (the partialcommit "extension", etc)
would be nice. In particular I'd like to better understand the memctx.
Is this something that was developed a while ago for mercurial core?
- What are the limitations of this? It seems this cannot work with
amend, can it? What about mq? These would be important limitations
which would be great to overcome if possible.

I'll explain all of this a bit later.

 

Despite all this comments I must say that I am very thrilled by this
and a bit impressed by the fact that this is already so functional.

The reason the progress has been so rapid (and I didn't really start working on this until last Friday) is that I spent the least amount of time as possible on the visual interface portion of this and instead focused on the difficult parts (integrating change selection into fileview without breaking its other users, tight integration with the status file list, making partial commits).  Qsci folding was the quickest way to get this feature "stood up".

I quite expect, in the end, to have at least one non-folding option for representing excluded changes.  If one of the non-folding options is clear enough to be understandable by first time users, I would be quite happy to ditch the folding approach entirely.  For today, it is our only option, but don't get too hung up on it.

Back to memctx.  This is a mechanism that has been in Mercurial for a really long time.  Its function is to make it possible to add new changesets (commits) to a repository without touching (or even having) a working copy.  Today it is used to import patches and is used by many of the vcs conversion tools.  Matt and the other hg hackers pointed me towards memctx as the solution to the general problem on Windows of not being able to predictably modify working copy files.  In particular, Visual Studio has a terrible tendency to hold files locked such that hgtk would abort during commits because it could not apply partial changes.  The memctx works around this by allowing you to supply the file contents explicitly.

If you look at partialcommit.py, uisetup() is the standard extension registration function that wraps an existing command (commit) and adds a new option (--partials PATCHFILE).  In our implementation, we pass control to the original commit function if --partials is not specified.  When a --partials patchfile is provided, we go through our memctx path.  This applies the patch to temporary copies of the working copy files, then uses the contents of those temp files during the commit.  I've had to modify those functions, which were written for the import command, to allow the commit to include files that are not modified by the patch (files which were modified, but did not have partial change selections).  That's what getfilectx() is for: memctx.commit() calls it repeatedly for file data.  Our version returns patched file data when it's available, then falls back to the working directory for the other files.  I just got all this working today.

What worries me is that memctx is old, and it is not oriented towards interactive commits.  There are a lot of features that have been added to commit since memctx was created (subrepos, bookmarks, merge status) that memctx is unaware of.  So I'll either have to add all of those features to our partialcommit function (and deal with all the typical problems that come from duplicated code), or I need to add them to Mercurial's memctx functions, or I need to back up again and figure out an alternative approach for feeding explicit file contents to the commit command.

Initially I thought memctx would not support amend, but I just noticed that amend is actually implemented within Mercurial as a memctx operation.  So it will be eventually possible to support amend with this feature; but it will likely take some work to figure all that out.

On the plus side, I think the memctx cleanly handles change selection for the copy/rename case, which was a frequent request for hgtk.

--
Steve Borho

[Angel Ezquerra]

Not necessarily. I I'd just like the "fold marker" to not be the usual
"+"/"-" within a square or circle, but a "checked / unchecked" box.

:-)

Despite these comments I am not really against folding. I am -1 for
folding _by default_. I am sure some (many?) people will like folding.
It may even grow on me with time...

>> A couple extra comments:
>> - I'd like to get a clearer picture of the actual implementation
>> details. I did not go through the revisions on a lot of detail, but
>> getting an overview of the design (the partialcommit "extension", etc)
>> would be nice. In particular I'd like to better understand the memctx.
>> Is this something that was developed a while ago for mercurial core?
>> - What are the limitations of this? It seems this cannot work with
>> amend, can it? What about mq? These would be important limitations
>> which would be great to overcome if possible.
>
>
> I'll explain all of this a bit later.
>
>>
>> Despite all this comments I must say that I am very thrilled by this
>> and a bit impressed by the fact that this is already so functional.
>
>
> The reason the progress has been so rapid (and I didn't really start working
> on this until last Friday) is that I spent the least amount of time as
> possible on the visual interface portion of this and instead focused on the
> difficult parts (integrating change selection into fileview without breaking
> its other users, tight integration with the status file list, making partial
> commits). Qsci folding was the quickest way to get this feature "stood up".
>
> I quite expect, in the end, to have at least one non-folding option for
> representing excluded changes. If one of the non-folding options is clear
> enough to be understandable by first time users, I would be quite happy to
> ditch the folding approach entirely. For today, it is our only option, but
> don't get too hung up on it.

OK, I wont :-)

Thanks a lot for the explanation. I did not know that memctx was a
builtin mercurial module. I am a bit surprised that it has not been
kept up to date though, specially if it is used by amend which in
theory should support all modern mercurial features, including
subrepos and bookmarks. IMHO it would make sense to bring the
shortcomings that you have found up on the mercurial mailing list. If
possible these should be fixed on the mercurial side.

Cheers,

Angel

[Steve Borho]

On Tue, Jan 1, 2013 at 5:15 AM, Angel Ezquerra <angel.e...@gmail.com> wrote:

On Tue, Jan 1, 2013 at 6:30 AM, Steve Borho <st...@borho.org> wrote:
>> I just tested this. It looks pretty good! I think _a lot_ of people
>> will be very, very happy when they discover this new functionality on
>> the next version.
>>
>> That being said, there are a couple of things that I'd like to
>> comment, some of which I already mentioned in the past:
>>
>> 1. I find the fact that this uses regular fold icons confusing. I
>> think this may lead people to not commit some changes by mistake.
>> Maybe a "checkbox" style icon would be better.
>
>
> I'll summarize that as -1 for folding.

Not necessarily. I I'd just like the "fold marker" to not be the usual
"+"/"-" within a square or circle, but a "checked / unchecked" box.

QSci has a few folding styles, but none of them use check boxes.  So using check boxes would have to be part of a larger non-folding design.

My plan is to get the current partialcommit extension fully functional with respect to subrepos, bookmarks, etc, and then to present this to hg-dev and ask for help about how to redistribute the code to make it more maintainable. 

--
Steve Borho

[Angel Ezquerra]

OK, seems like a good plan. You won't come to the London sprint, will you?

Angel

[Steve Borho]

There's a small chance I'll be able to make it   I have to make monthly trips to Helsinki the first half of 2013.  it's possible the sprint will be right before one of those trips, in which case I can use London for a convenient 3-day layover.

--
Steve Borho

[Angel Ezquerra]

That would be really awesome, since it is quite likely that I will also make it. It would be great to finally meet you in person :-)

Angel