feature: worktree with stage view

[antis81]

Goes on with PR #61

Basic UI:

• [x] Add a view to show staged changes
• [ ] Add an embedded commit "dialog", with commit, sign-off and amend functionality
• [x] Implement filters for the staging area, following "single-source-principle"
• [ ] A bigger topic is sync'ing the file status (nothing new, eh?). For the first run, I would stay away from this and just rebuild everything from scratch on refresh (see #32). Means ... simply call "repositoryChanged" in the main view manually.

Actions:

• [x] Add actions for context menus and such
• [ ] Implement functionality to be able to actual stage / unstage files

Cleanup and usability:

• [ ] Show folders in the stage view only, when affected
• [ ] Add move and hide functionality to header columns in tree views

Diff / File View:

• [ ] Show the Diff for the selected file only (no sequence of files)
• [ ] Implement a hunk and line selection mechanism (aka "line-wise staging"); see also GitX commit dialog for this

Internal (refactor / redesign):

• [x] move to QSortFilterModel based filtering (maybe sorting e.g. for file mtime, size, ... also in the future)

[scunz]

In theory, nothing speaks against using a QSFModel - at least not as long we don't feel the pain of memory pressure - but I don't see a particular memory pressure where we could run into.[1]

Do you want to use it for sorting, filtering or both? The model on which the WT is based already has filtering mechanisms built in. That's not mandatory to keep. However, most of the time I prefer this approach, because it's actually simple to implement and can easily be expanded if memory pressure becomes an issue - As opposed to the QSFModel, where you're required to always have the full set of data available and just select a part of it for view.

[1] actually, i do, but i'd rather like to ignore that :smile:

[antis81]

Ok then. Let's assume we don't care about memory right now :smile:. In general it works and so I commit for discussion / review.

Do you want to use it for sorting, filtering or both?

Currently the QSortFilterModel is used for filtering only.

I left the core code intact in respect to not change too much at a time.

• The worktree filter is saved and fully functional. This moved to the IndexWidget main view.
• The filter itself moved completely to QSortFilterModel, except of the checkbox functionality
• Directories are currently not displayed correctly, as they have no status. There's still a bug somewhere I just don't see. :frowning:.

Two questions that came up:

1. Can we replace the WorkingTreeFilter enum with the real thing now? We'd have to add index filters and that's basically a subset of Git::Status. We already discussed that and I left it, making as small changes as possible.
2. Do we need the visible flag in the WorkTreeItemAbstract? I think that this is done perfectly now by the filter model.

And ... I couldn't help: Memory footprint of MGV+Qt repo is roughly about 350 MB and without building the working tree it is ~1/2 (150 MB). Regardless which method we use. I know it is not very representative though.

[antis81]

Ok, found and fixed the "directory bug". It is because the "makeVisible" routines interfere with the QSFModel. I removed everything concerning that and it looks great so far.

Concerning the WorkingTreeFilter: I'll leave it there for now and just add an additional state InIndex. That is set additionally, if the file entry has changed in any way within the index (it is staged). Oh, I forgot to mention that the WorkingTreeFilter changed to WorkingTreeFilters - means it can have several states simultaneously and allows us to use it like stated above.

[antis81]

Tada :heart: - we have a first working implementation of a stage. Ok ok, no actual actions are possible yet, but the filters go great. Please review.

[scunz]

I'm a bit bewildered by the first commit (because I don't see a reason for a second tree and a commit message in the main UI).

The remainder looks promising to me. The reasoning for your further changes is sound and sane. However, I've seen a few things in the diff that look strange at first glance. But I'll have to investigate to actually judge on those.

Wrt. WTF_-enum: Indeed your status changes seem to make it superfluous and we should get rid of it.

How do you plan to visualize a file that has properties like this: It exists in HEAD, exists in INDEX but it's SHA differs from HEAD and exists in WorkTree and that file's SHA differs from HEAD and INDEX:

git init .
echo "A" >test && git commit test -m"Initial" # HEAD    = "A\n"
echo "B" >>test && git add test               # INDEX   = "A\nB\n"
echo "C" >>test                               # WORKDIR = "A\n\B\nC\n"

I have that situation very regularly...

And ... I couldn't help: Memory footprint of MGV+Qt repo is roughly about 350 MB and without building the working tree it is ~1/2 (150 MB). Regardless which method we use. I know it is not very representative though.

Yes, i never said that is actually optimized. But what I rather wanted to say: With the QSFModel we lose the ability to say: "Oh, the user wants to see only the modified entries. Let's not put all others into the memory".

[antis81]

Thanks for reviewing so quick and for that comment. I was also not sure first, where this will lead. I never used the QSFModel before. But, actually it turns out to be the right direction to go on.

Wrt. WTF_-enum: Indeed your status changes seem to make it superfluous and we should get rid of it.

Yeah, now I see this also. But it's a good thing to go stepwise. I think we both are more clear now about the situation and how to solve it. Concerning the example, I think, this would actually show like expected (calling for a bunch of unit tests :smile:). Anyways, we are much better off using the Git::Status flags now that we have them.

Ah "what shall's" ... I put the subtasks on top.

[antis81]

The replacement of the filter enum requires another change in GitWrap: The Git::FileUnchanged status is 0 and thus not comparable like the other flags. I'm going to reassign it to (1u << 15) and introduce another flag FileInvalidStatus = 0. Putting this in another small PR as this has dependencies on methods relying on the 0 in case of an error.

This is the "flu" you saw comming - :100: points.

[antis81]

Once again, sorry for a mixed commit, but I'm not sure how to divide it into smaller ones. This one introduces the needed change in GitWrap. Nevertheless, I think it was worth the effort.

Btw.: This also replaces the "All Filters" checkbox with two normal selections. I think this is even better having usability in mind.

I'm not sure, if we should use members for the WorkingTreeFilterModels instead of casting from the view->model(). What do you think?

[scunz]

This is the "flu" you saw comming.

Not exactly. I was actually sick for a few days before easter. But nice to see that my intuition didn't play tricks on me :smile:

I'm not sure, if we should use members for the WorkingTreeFilterModels instead of casting from the view->model(). What do you think?

I think, that I don't like any dynamic_cast[1]. As such, I would have commented sooner or later on that point. First off: In that context we do exactly know what type we have. We can static_cast without ever having a problem. However, the reason I did not comment on it, is that it will vanish anyway. A ViewContextData derivat will have to store the pointer to the WorkingTreeFilterModel and then nothing's speaking against the actual types right from the start.

[1] I'm so scared of the fact that less than half a percent of all C++ programmers know what a dynamic_cast is supposed to do and how it works. Consequently, I feel urged to explain it and ask: "Is this really your intention?". No one ever answered that with yes - and only very few kept on arguing. But those were those that would have been better off if they targeted something like logo as their primary language...

[antis81]

Not exactly. I was actually sick for a few days before easter. But nice to see that my intuition didn't play tricks on me.

Oh sorry - misunderstood. Fortunately the big wave is over.

I think, that I don't like any dynamic_cast ...

Me neither. That's why I asked and glad you have a good idea to get it to the right place. I wouldn't say, I fully understand how it works - but I know for sure what it does and that it is better to avoid that.

To the logo comment: Yep, that's the spirit. I'd even say those don't like it, when somebody else sees further than themselves. Another one to talk for days ... but ... uh well. Case closed :smile:.

[scunz]

I wouldn't say, I fully understand how it works [...]

Technically spoken, it's the same expensive operation like git_merge_base. There's static meta data on all classes which forms a directed (mostly) acyclic graph, which represents the inheritance hierarchy of the C++ classes. This graph can become cyclic if multiple, virtual inheritance is involved. From a mathematical point of view a dynamic_cast is: start at the class-info we currently are, then walk the graph backward and for each node travel forward again, until we find the destination class-info. Sometimes there are multiple correct traversals (for which the C++-Spec defines where to go). When we're at the destination class-info we walk back to the original again and on that way sum up the memory alignments in each iteration. Finally, the this pointer is adjusted by that value: i.e. this = route_found ? ((char*) this - 20) : NULL.

A static_cast is actually the same, just that the calculation is done at compile time and if route_found is not true, a compiler error is raised.

[antis81]

Cool, thanks for the explanation :+1:. So in short the main difference is, that a dynamic_cast can have "bad influence" to runtime when used in many iterations and the static_cast has a constant runtime.

[scunz]

Exactly. Plus that the dynamic one fails at runtime and the static one fails on compile time (and isn't 100% safe at run time)

[antis81]

@scunz restructured the subtasks. Btw. could we refer to separate issues to not "spam" this PR? Because it is getting a bigger thing.

Concerning the CtxMenus (actions in general) I have started implementation work (which I'm just about to partly commit), mainly inspired by the RepoTree to get an idea of how it works now. And again ... I would prefer a little different approach using a QStyledItemDelegate instead of the CtxTreeView. The reason for that is, it already has all we need to show the context menu (in clear text: the model index and position). In addition, we could use it to get better (= central and more flexible) control over the item data, instead of implementing the data role within the TreeItem itself. I really like the TreeItem from MsPiggit :heart:, that uses a variant map to store data. It actually behaves like the DataRole, but instead uses strings as keys and slims the code a lot. But ... we can do that after the basic functionality is working, as this has probable dependencies to the core and heaven libs.

What I'm thinking about is: Having a menu map referencing to the items type (which could be a string). So ... it should be possible to call CtxTree.menuForItem("the_key"); and get the matching menu. Off course we setup the menu using CtxTree.setMenuForItem("the_key", theHeavenMenu, theWidgetDoingTheActionStuff);

Or even more generic ... let libHeaven provide an inherited QItemView with that functionaliy?

Anyways, I commit the basic things and we see later where this is going.

[scunz]

@scunz restructured the subtasks. Btw. could we refer to separate issues to not "spam" this PR?

Answering this thing first, as it's probably more important. I don't see a reason for creating an artificial network of issues / pull requests for what is actually one feature. Organizing things in GH is very limited. As long as we're stuck with GH, we must be satisfied with what GH provides. That is: Issues, Milestones, PRs and Sub-Tasks.

Issues are very much like a thread on a forum; They are sequential, not hierarchical, which is against any modern form of communication. Asides always clutter up the main issue (A mailing list thread doesn't do that, because individual branches can have a life time of their own and if the need arises can be turned into a brand new topic). Milestones are meant for the big picture and that's what they should be used for and where they are actually good for. We're doing that already and everything is fine here. PRs in GH are actually issues with attached code. As such they suffer from the same problems as Issues do. Additionally commits, references and discussions are intermixed in the PR view, which makes the overall look even more cluttered. The usage of subtasks is very limited, too. These can be used for tracking completeness of a PR - But nothing more. (I think that his was their intention, just to be fair to the GH fellows).

The automatic linking of Issues and PRs is a two-sided sword. How many bidirectional links to old libgit2 Issues do we want when we are talking about whether they affect us or not? :-)

There's one thing to which I'd strongly agree though: This organizational stuff is spam here. It belongs somewhere else, but it's not stuff for an Issue either. Organizational stuff like this should probably be discussed on a mailing list.

Because it is getting a bigger thing.

This again, is an artificial boundary. We should not make things big by default. I think, the big-bang change in libHeaven was an exception (it might actually have been justified to get that big through the impact it had) and it should stay an exception. We should rather not let things get that big. And indeed in every category that you put into the root description of this PR, I really see a different pending PR. I.e. what do changes to the diff view actually have in common with the ability to stage?

Additionally comes the problem of cross-repository dependencies, which can only be solved with discipline and memory. But, frankly, I've yet to cross way with a human being that has infinite discipline and memory.

[scunz]

And again ... I would prefer a little different approach using a QStyledItemDelegate instead of the CtxTreeView. The reason for that is, it already has all we need to show the context menu (in clear text: the model index and position)

In which aspects do the feature sets of TreeViewCtxMenu and QStyledItemDelegate overlap? The context-menu request signal of TreeViewCtxMenu has the signature void contextMenu( const QModelIndex& index, const QPoint& globalPos ) Why exchange it with another implementation that provides index and position? Beside the fact that from looking at the QStyledItemDelegate docs, I don't see the place where you would hook that up?

I really like the TreeItem from MsPiggit :heart:, that uses a variant map to store data. It actually behaves like the DataRole, but instead uses strings as keys and slims the code a lot.

I really dislike this idea. What is the purpose of reimplementing everything that the "deprecated" QTreeWidget can already provide, just with a slightly slower String-To-QVariant map instead of the Int-To-QVariant map (which already is way slower than what we're doing with QAbstractItemModels).

Also, I fail to see why giving QLatin1String( "CtxMenu" ) would be preferable to TreeViewCtxMenu::MenuRole?!?

What I'm thinking about is: Having a menu map referencing to the items type (which could be a string). So ... it should be possible to call CtxTree.menuForItem("the_key"); and get the matching menu. Off course we setup the menu using CtxTree.setMenuForItem("the_key", theHeavenMenu, theWidgetDoingTheActionStuff);

1. No single widget will handle the actions for a file
2. Not even a single module will handle the actions for a file
3. Actions do extremely depend on the operations available to that file. You don't want to multiply all of that and provide n .hid files, do you? It starts with the case that both stage and unstage might need to be available at the same time.

[antis81]

In which aspects do the feature sets of TreeViewCtxMenu and QStyledItemDelegate overlap?

See here: http://qt-project.org/doc/qt-4.8/qabstractitemdelegate.html#editorEvent

The basic idea is the same, just that the delegate can be used more generic than the QTreeView.

Sorry for being so unclear. It is really a big topic and should be a separate issue / PR like "Context Menu creation in (Heaven) Views". There's like so often a small problem leading to big ideas.

I have something in mind like: Inheritance: MGVGenericItem<-MGVWorkingTreeTreeAbstractItem<-MGVFileItem

And then calling the menu like so:

void IndexWidget::contextMenuWt( const QModelIndex& index, const QPoint& globalPos )
{
    MGVGenericItem * item = static_cast<MGVGenericItem *>( index.internalPointer() );
    if ( item == 0 )
        return;

    Heaven::Menu * m = myDelegate.menuForItem( item );
    if ( m != 0 )
        m->showPopup( globalPos );
}

In general, the delegate could contain a QMap<MGVItemType, Heaven::Menu*> mMenuMap that provides the menu.

Ok, I see the rest of this discussion is better off in a separate issue / PR.

[scunz]

See here: http://qt-project.org/doc/qt-4.8/qabstractitemdelegate.html#editorEvent

I still don't see where a connection between those two functions is. We're talking about the event that is triggered by either right clicking an item or by pressing the context-menu-key while item delegates (esp. the function you've posted above) are about overlaying lineedits / spinboxes / sliders on top of a item in the view.

Why can't the IndexWidget itself contain such a map (if such a map is required at all)? After all, there are just two types of Items we could expect: directories and files. The type of those is already available through the model. The real functionality has to go to merge logic anyway. Here we will (in the longer term) require some kind of conditionals; but that's out of scope here.

It is really a big topic and should be a separate issue / PR like "Context Menu creation in (Heaven) Views". There's like so often a small problem leading to big ideas.

No, let's please not defer this one. I see the ability to trigger actions in the Working-Tree as essential to partial staging. This is the first place, where we need it and we should now think about how to do it right. We can stack the bricks now, but if we don't learn how to mix concrete now, we won't be able to insert the concrete when the roof is already on top of the building :-)

Ok, I see the rest of this discussion is better off in a separate issue / PR.

How will we ever reach consensus if we defer any open questions to a "separate issue"? :smile: What you're proposing here is different from what I have planned and partially started. I mind neither discussion nor conceptual changes per se. Among your suggestions were many that caused me to rethink parts of this whole project (Just think for the submodule stuff). I don't understand why you withdraw your ideas as soon as something doesn't fit in in the first place. Maybe we can make it fit, if there's a reason to do so.

[antis81]

I see. Maybe it is too early to even think about something like that. So I'll finish implementation of what I have started without changing anything conceptual at this place.

[antis81]

@scunz Hm, I experience a little strange segv when accessing any TreeItem data (e.g. item->isDirectory()) and couldn't figure out what's going on so far. The WTAbstractItem seems to be valid, but it actually isn't. Is my debugger playing tricks on me?

After that is clear, I have another small question before going on with the implementation, we have two ways to access the tree item within the slots from the tree view:

1. Inherit WTAbstractItem from QObject and set it as "menu.setActivationContext"
2. Use "this->currentIndex().internalPointer()".

Both variants need to cast either the QObject* or the void* as WTAbstractItem to access item data. My "stomach feeling" says we should take currentIndex(). Which way to go?

[scunz]

@antis81, sounds like a dangling pointer. I can't see it from the diff, it's too much code; but I'm currently rebuilding KDE, so no nice GUI... Will download and check it out ASAP... :-)

As for your question, that is actually a good one. Must think a while about that. I think that 1. is the right way to go, but I'd prefer a less memory exhausting option. Might be that avoiding a mess here will require an extension to the activation-context stuff. I need activation contexts per file/directory, too (for the Custom-Command stuff); maybe we can make them temporary? I really gotta think this through deeply first. Until that's through, we can probably go either way. So I'd say: If it doesn't cause trouble now, go ahead with 2. Just do the casting part inside the model and call it like: model->indexToObject( this->currentIndex() );

[scunz]

After looking around a bit (still without even seeing the segv), I think it's absolutely logical:

• The QModelIndex you're dealing with in contextMenu belongs to the WorkingTreeFilterModel.
• Indicies in that model don't point their data to the WTAbstractItem tree.
• You need to map from the QSFModel to the real model, first.

[scunz]

i.e. like: (untested):

QModelIndex deeplyMapToSource( QModelIndex current ) {

    while( current.isValid() ) {
        QAbstractProxyModel* apm = qobject_cast< QAbstractProxyModel* >( current.model() );
        if( !apm ) {
            return current;
        }
        current = apm->mapToSource( current );
    }

    return QModelIndex();
}

[antis81]

Thanks for that hint - think you're right and found it :clap: :+1:. I'll try it out and report back.

EDIT: YEP - that's exactly that. Actually I thought about that before but somehow forgot it - strange human mind - or simply too much code :coffee: :smile:.

[antis81]

We should put the index mapping into the TreeViewCtxMenu base class? That's a cross-module-change though :anguished:.

[scunz]

We should put the index mapping into the TreeViewCtxMenu base class

I think we should only do this, if we want to force every consumer of a TreeViewCtxMenu to use a QSortFilterProxyModel, which I'd like to avoid. The mapping is clearly just required because the WorkingTree stuff now uses Proxy-Models.

[antis81]

@scunz Phew - even without other changes this is still a big topic :sweat:. But it's fun anyways :smile:.

Two (future) ideas:

• I am wondering, if we could / should template-ize the TreeViewCtxMenu class to make it acting on the correct model type (in this case WorkingTreeModel). Maybe we can even add the tree items it manages like: TreeViewCtxView<WorkingTreeModel, WorkingTreeAbstractItem>.
• And it is worth noting: The checks on model index and item pointers have to be repeated in each action-slot. We could and should optimize this a little in the (nearer) future in respect to writing more actions.

Apart from that, the menu is working on the correct items now and we come to the :heart: of the :chestnut: - accessing the index for staging / unstaging and updating the models/views afterwards.

EDIT: Ups, overlooked your previous comment. Hm, a quick :bulb:: Maybe we could introduce another class with these changes like FilteredTreeViewCtxMenu inherited from TreeViewCtxMenu?

[scunz]

I am wondering, if we could / should template-ize the TreeViewCtxMenu class to make it acting on the correct model type (in this case WorkingTreeModel)

Might be that Oliver Goffart's work for templated QObjects might hit the real world in Qt 5.2/5.3 or such :-)

And it is worth noting: The checks on model index and item pointers have to be repeated in each action-slot. We could and should optimize this a little in the (nearer) future in respect to writing more actions.

When everything's alright, the actions won't ever know of a model index at all.

Apart from that, the menu is working on the correct items now

Alright. That's enough for now :-) I will make it plug-able later.

[antis81]

With these changes, it is actually possible to stage a file - finally :+1:.

Needs PR macgitver/libGitWrap#15

NOTE: Unfortunately, "unstaging" does not work by simply removing an entry from the index. This will remove the file entry from the index (I was brave and tried it on MGV repo :innocent:). So index.removeEntry() works like git rm --cached a_file.txt. Don't have any idea yet how to get it right. Do you?

[scunz]

The new code looks okay; both here and in the dependent PR in libGitWrap. Do you think, this is done now? :-)

[antis81]

To be honest, it is not really done. But I really want to see it in development branch :smile:. Maybe we could merge here and go on in separate PR's at the right places.

Btw.: Sorry for my lazyness, but weather situation simply didn't allow for progging :laughing: :+1:.

[scunz]

To be honest, it is not really done. But I really want to see it in development branch. Maybe we could merge here and go on in separate PR's at the right places.

Feel free to go on like that.

Btw.: Sorry for my lazyness, but weather situation simply didn't allow for progging.

Has the weather in the south been that much better? Here we've had just one nice day, recently. I used to live in your area for approximately 2 years (actually east of the big city), i never really enjoyed the hot climate over there. I mean, it wasn't bad at all, but so much heat without much moisture... Contrarty to that, I really enjoyed the years at Friedrichshafen :-) After all, this isn't paid work - I'm glad anybody other than me is caring for all this weired stuff I do :-) Esp. when I'm so busy doing so many other things

[antis81]

Yeah, the climate is really somewhat warmer in this area - especially if you're born in good old S(ch)auerland :umbrella:. Summer is here :sunflower: (25°C yesterday) - spring season is skipped this year :smile:.

I really like to support this project including all the discussions, reviews, PR rejects and everything. All this (partly hard work) is resulting more and more in a great UI and developing experience. Combining the projects was a cool idea. Also we cannot see "shiny and fast progress" on the surface, there's a lot of good stuff going on under the hood. This allows us to just do the right things and get it really solid. And all that is somehow still fun - just great!!!

Ok, I'll merge here in favor to get the basic idea and a little functionality and more of the good spirit into MGV. But, to make the stage somewhat like "feature-complete", lots of stuff is still missing.

[scunz]

Yeah, there wasn't very much trace of spring this year, indeed. Anyway, thanks for the nice words.

I guess it's good to have this merged now, from here onwards we can go in smaller separated steps.