Skip to content
This repository was archived by the owner on Jun 3, 2024. It is now read-only.

Sc/refsview#99

Merged
scunz merged 80 commits intodevelopmentfrom
sc/refsview
Mar 21, 2015
Merged

Sc/refsview#99
scunz merged 80 commits intodevelopmentfrom
sc/refsview

Conversation

@scunz
Copy link
Copy Markdown
Member

@scunz scunz commented Feb 11, 2015

An alternative approach to improve the RefsView.

This is based upon ngf/model-update and includes the compile-fix commit, which should be removed from the branches by rebasing them to development.

This approach doesn't consider stashes and namespaces yet. The support for stashes is lacking in RepoMan and probably in GitWrap. For namespaces I'm simply not sure how to display them correctly. The basic problem is, that each namespace could have both branches and tags (as well as other namespaces, in fact).

But we're actually having the same problem with remotes, where we should show a separate tree for tags.

@antis81
Copy link
Copy Markdown
Member

antis81 commented Feb 12, 2015

Here's what it looks like in Ubuntu 👍
mgv-ref-tree-sc

I just compiled and didn't look deeper into this code yet. Btw. Compared to the other branch, drawing with current code is actually performing faster, which you can easily see by dragging the splitters.

For the visual appearance, I have some questions / notes:

  • I think, the icons are "cluttering" the view. Can you try putting them only to the headers?
  • (Icons for RefTreeNodes should be kept, I think.)
  • I really like to have the colors back. But, it's not finished, is it?
  • Same for the bold font, that marks the active branch.

@scunz
Copy link
Copy Markdown
Member Author

scunz commented Feb 12, 2015

Btw. Compared to the other branch, drawing with current code is actually performing faster, which you can easily see by dragging the splitters.

Which code are you talking about? this code omits your reading of the repository from disc on each drawing, so it is faster.

I think, the icons are "cluttering" the view. Can you try putting them only to the headers?
(Icons for RefTreeNodes should be kept, I think.)

No, these icons don't make any sense in the header. And having an icon for all items finally removes that annoying cluttering that made it impossible to spot the actual hierarchy. Everything is now aligned as it usually is in a tree.

I really like to have the colors back. But, it's not finished, is it?

No, this isn't done. The underlying data model doesn't carry that information and I don't want to reintroduce the code that reads this information from disc on every drawing operation.

Same for the bold font, that marks the active branch.

Same cause, same solution. The model has to carry this data, determined once during a refresh and updated with events accordingly.

@antis81
Copy link
Copy Markdown
Member

antis81 commented Feb 12, 2015

Which code are you talking about? this code omits your reading of the repository from disc on each drawing, so it is faster.

Yes, this is exactly what I was thinking about :). There's some other aspects to: i.e. you kicked the sort model layer. But this is the main point. It's not too bad, as the lookup is always done on the items drawn on screen (which is always <50). Ah ... I'm not writing any more, until I reviewed the code :).

... And having an icon for all items finally removes that annoying cluttering that made it impossible to spot the actual hierarchy. Everything is now aligned as it usually is in a tree.

I'll adapt to this and see what future brings. 😄

The underlying data model doesn't carry that information and I don't want to reintroduce the code that reads this information from disc on every drawing operation.

Happy to hear that. We need to discuss the RM::Repo::heads() stuff, I introduced in libMacGitverCore -> PR 22/23 and make it right :). I.e.: Keep the HEAD-ref as a separate instance - out of the collections. Lookup and compare would be fast, furious and even cool :).

In current implementation that means:

QVariant RefBranch::data(int role) const
{
    Q_ASSERT(mObject);

    switch (role) {
    case Qt::FontRole:
        if( isCurrentBranch() )
        {
            QFont f;
            f.setBold( true );
            return f;
        }
        #endif
        break;

    case RefItem::RowBgGradientRole:
        if ( equalsHeadId() )
        {
            QColor back = isCurrentBranch()
                          ? QColor::fromHsl(35, 255, 190)
                          : QColor::fromHsl(35, 255, 190).lighter(130);
            return back;
        }
        #endif
        break;

    case Qt::EditRole:
        return object()->name();
    }

    return RefItemObject::data(role);
}

// spice it up with some private salt & pepper :)

const RM::Ref* RefBranch::findHEAD( const RM::Repo* repo ) {
    // this can be optimized in libMacGitverCore, by providing a RM::Repo::head() method
    // Anyways, this code will work!

    RM::Ref* HEAD = NULL;
    const QString headStr = QStringLiteral("HEAD");
    foreach ( RM::Ref* candidate, repo->heads().childObjects<RM::Ref>() ) {
        if ( candidate.fullName() == headStr ) {
            HEAD = candidate;
            break;
        }
    }

    return HEAD;
}

bool RefBranch::isCurrentBranch() const
{
    const RM::Repo* repo = mObject->repository();
    const RM::Ref* HEAD = findHEAD( repo );
    // Q_ASSERT( HEAD );

    return HEAD && HEAD->symbolicTarget() == object()->fullName();
}

bool RefBranch::equalsHeadId() const
{
    const RM::Repo* repo = mObject->repository();
    const RM::Ref* HEAD = findHEAD( repo );
    // Q_ASSERT( HEAD );

    return HEAD && HEAD->id() == object()->id();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can safely remove this class!

This was referenced Feb 12, 2015
@scunz scunz force-pushed the sc/refsview branch 2 times, most recently from 4209cc2 to deefcc5 Compare March 2, 2015 15:44
@antis81
Copy link
Copy Markdown
Member

antis81 commented Mar 6, 2015

This needs rebasing due to the macro changes in GitWrap. Otherwise, compiler will complain about a missing semicolon.

@scunz
Copy link
Copy Markdown
Member Author

scunz commented Mar 6, 2015

This needs rebasing due to the macro changes in GitWrap. Otherwise, compiler will complain about a missing semicolon.

Yepp. Though my local branch still has that commit around - I never start to develop on the "development" branch; not even for fixes :)

@scunz
Copy link
Copy Markdown
Member Author

scunz commented Mar 6, 2015

Rebase done :)

@antis81
Copy link
Copy Markdown
Member

antis81 commented Mar 6, 2015

... I never start to develop on the "development" branch; not even for fixes :)

Maybe that's why lg2 renamed their "development" branch back to "master" 😄.

Rebase done :)

It's funny to see the website updating while writing in this box 😄. Thanks!

@antis81 antis81 mentioned this pull request Mar 7, 2015
2 tasks
@antis81 antis81 mentioned this pull request Mar 16, 2015
16 tasks
@scunz scunz force-pushed the sc/refsview branch 2 times, most recently from d447993 to 8e49c1a Compare March 17, 2015 20:55
antis81 and others added 26 commits March 21, 2015 00:29
This is also valid for a renamed reference!

Minor downside: When a scope/namespace has no more children, it will still stay visible.
As far as _behaviour_ is concerned, we don't want the headlines enabled.
So that they cannot actually even be focused. However, this is certainly
not what we want to draw.
scunz added a commit that referenced this pull request Mar 21, 2015
@scunz scunz merged commit 2418730 into development Mar 21, 2015
@scunz scunz deleted the sc/refsview branch March 21, 2015 17:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants