git add -p toolview

[Jonathan Verner]

Hi,

thanks for the quick and kind response :-)

> Do what fits you best, but once we merge I'd like to keep a clean history.
> I.e. try to create separate individual commits that make up an atomic change,
> while being as small as possible.

I tried to do that already, but I will need to do some cleanup (e.g. some of the
commits miss an include which is only added later on, when I realized I
forgot to add it).

> Below e.g. you are mentioning changes to
> some of the existing libraries - these could be done in separate patches then
> e.g. to one bigger patch that introduces the new toolview itself.

Makes sense, I will split them out into separate merge requests.

> Hmm good question! This is all handled in CodeHighlighting (kdevplatform/
> language/highlighting/codehighlighting.cpp) which hasn't been touched in years
> and could quite probably use some cleanup.

I've been going over that file trying to understand it, but somehow
got lost. I got as
far as guessing the actual coloring of the text in the editor is done on lines
589, 590 and 594 in applyHighlighting by setting an attribute on the
range which
gets propagated to KTextEditor. However, then I don't see why I don't get the
results back from KTextEditor. I could probably add a public method to
the CodeHighlighting
style to give me an instance of CodeHighlightingInstance where I could
get to the ranges.
Would that be o.k.?

> But potentially you would be better of with creating two temporary files for
> the A/B version. Then we'd need to find a way to reliably create a parse job
> for those using whatever language that is used for the source file, and then
> ensure the same compile flags etc. pp. are also used.
>
> This would ensure that both versions get highlighted properly. Otherwise you'd
> only ever have one correct highlighted version (the newer one, B), or?

Yes. In the current implementation, for simplicity, I just highlight
the part of the diff which
shows the work-tree (i.e. newer) version.

In general, the problem with the older version is that it might depend
on other files which
have changed in the meantime, so to get a completely correct
highlighting, one would
need to checkout the full repo at the older version, which I don't
think is viable.
But if we give up this option, it would still make sense to make a
best effort attempt
and live with the fact that some highlighting will be lost.

Anyway, since I didn't manage to get the semantic highlighting working
even for the
simple case of the work-tree version, I left this for later :-)

O.K., so, moving forward, I will try to split the merge requests into
separate ones dealing with the library
changes and a single request adding the toolview on top of them.

Best,

Jonathan

On Thu, Apr 30, 2020 at 4:21 PM Milian Wolff <mail at milianw.de> wrote:
>
> On Donnerstag, 30. April 2020 14:51:27 CEST Jonathan Verner wrote:
> > Hi,
>
> Hey!
>
> > I've been working on a toolview to stage and commit changes to git.
> > The motivation for this follows below. I am now in a stage where the
> > toolview is already useful and would be happy for some feedback. I've
> > created a merge request on invent.kde.org:
> >
> > https://invent.kde.org/kde/kdevelop/-/merge_requests/128
> >
> > (or do I need to create a review request on Phabricator?).
>
> Gitlab is fine. And thanks for your contribution! The screenshots look very
> promising already. I know that quite a few people have asked for this. I'm a
> heavy user of `git add -p` on the command line myself too.
>
> > Currently I
> > included the full history, so that the merge request has quite a few
> > commits. I am not sure if this is prefered or whether I should rewrite
> > the history squashing the commits to reduce this number.
>
> Do what fits you best, but once we merge I'd like to keep a clean history.
> I.e. try to create separate individual commits that make up an atomic change,
> while being as small as possible. Below e.g. you are mentioning changes to
> some of the existing libraries - these could be done in separate patches then
> e.g. to one bigger patch that introduces the new toolview itself.
>
> <snip>
>
> > Questions
> >
> > ---------------
> >
> >
> > - Is there a way to get to the results of the semantic highlighting
> > (ideally in the form of a list of attributes for given source lines)?
> > I didn't find out how to copy semantic highlighting over to the diff;
> > right now I open the source document, create a moving range and use it
> > to access line attributes which I then copy over to the the diff;
> > however, this only gives me basic syntax highlighting (even if the
> > source document is already fully semantically highlighted...).
>
> Hmm good question! This is all handled in CodeHighlighting (kdevplatform/
> language/highlighting/codehighlighting.cpp) which hasn't been touched in years
> and could quite probably use some cleanup.
>
> But potentially you would be better of with creating two temporary files for
> the A/B version. Then we'd need to find a way to reliably create a parse job
> for those using whatever language that is used for the source file, and then
> ensure the same compile flags etc. pp. are also used.
>
> This would ensure that both versions get highlighted properly. Otherwise you'd
> only ever have one correct highlighted version (the newer one, B), or?
>
> Cheers
>
> --
> Milian Wolff
> mail at milianw.de
> http://milianw.de