Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix RemoteObjectTree branch folder nodes sometimes becoming bold #5686

Merged
merged 1 commit into from Nov 7, 2018
Merged

Fix RemoteObjectTree branch folder nodes sometimes becoming bold #5686

merged 1 commit into from Nov 7, 2018

Conversation

amaiorano
Copy link
Contributor

Fixes RemoteObjectTree sometimes resulting in font bolding for branch path nodes upon switching repo.

Screenshots before and after (if PR changes UI):

This is what can happen:
image

Note that "amaiorano" should not be bolded. This happened because the previous repo's 2 root nodes had the second node bolded, not the first; upon switching to this repo, it recycled the 2nd node for the branch path "amaiorano", but left it bold. This happened because the font reset for all nodes was removed as an optimization in cd68a51. This change brings it back, but in an optimal way.

What did I do to test the code and ensure quality:

  • Tested opening different repos, and in particular the one that caused this problem to show up.

Has been tested on (remove any that don't apply):

  • Windows 10

@amaiorano amaiorano mentioned this pull request Nov 3, 2018
Merged
amaiorano
amaiorano commented Nov 3, 2018
View reviewed changes
GitCommands/Settings/AppSettings.cs Outdated
}
}

private static Dictionary<FontStyle, Font> _fontByStyle = new Dictionary<FontStyle, Font>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have liked to have used an array indexed by FontStyle, except that FontStyle's values are bit masks. I could still use them, but we'd have a sparse array of size 8 where only 1, 2, 4, and 8 indices would be used. It would be faster than using a dictionary. Or we could also convert bit position to index, but that requires looping to count number of right bit shifts. Thoughts?

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@amaiorano
Copy link
Contributor Author

How many different fonts are we talking about?

So FontStyle from System.Drawing looks like this (currently):

    [Flags]
    public enum FontStyle
    {
        Regular = 0,
        Bold = 1,
        Italic = 2,
        Underline = 4,
        Strikeout = 8

In my case, Regular is handled separately, returning Application.Font, while Bold, Italic, Underline, and Strikeout are created lazily if requested. So at most 4 more Font objects will get created. My current use case is just for the Bold, but for my Submodules ROT change, I will use Italic as well.

@amaiorano
Copy link
Contributor Author

Another thing worth mentioning is that we could also add similar functions for the other fonts in AppSettings - FixedWidthFont, CommitFont, and MonospaceFont - if we need styled versions of these in GE. Also, this might be a step in the right direction for being able to fix the problem of having to restart GE after changing fonts. If everything uses these centralized fonts - even for style - then we have one place to change, and we can use some kind of callback/reload mechanism.

amaiorano
amaiorano commented Nov 3, 2018
View reviewed changes
GitCommands/Settings/AppSettings.cs Outdated
set
{
SetFont("font", value);
_fontByStyle.Clear();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This here clears the cache when we change Font (through Settings), so future calls to GetFontByStyle will do the right thing. Of course, presently we don't have a mechanism to reload UI based on font change, but this is a step in the right direction, I believe.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@amaiorano
Copy link
Contributor Author

Ok. So frankly a dictionary would be best. You have a few named fonts not a ton. Then a few variances based on style. So why not make the key a composite of string and a fontstyle. Yes. We do need a font reset/load functionality.

I hear you, but I feel like doing this now would be making us pay for something we don't use. Having to hash the name and enum value will be more costly than just the enum value in terms of dictionary lookup. If we see that it would be useful to have styled versions of the other fonts, then sure, we could do as you suggest. In any case, this would be a detail of the implementation, not exposed via the public API. For example, we'd add GetMonospaceFontByStyle, and internally, it and GetFontByStyle could use the same dictionary keyed by font name + style enum value. Having said that, clearing the cache would be more difficult since changing a single font shouldn't clear the entire cache. So maybe we'd really just want separate dictionaries for each font, in which case, we're back to what I've done here.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@amaiorano
Copy link
Contributor Author

You don't have unique fonts per style. You have fonts with styles and they are linked to where they are used.

I don't understand what you're trying to say. I know that conceptually we have fonts with styles, but concretely, we instantiate unique Font objects per font per style.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email •
edited

@amaiorano
Copy link
Contributor Author

My point is we have a store of fonts that get lazy loaded based on use. The dictionary has a location key that says it is font x for commit header or whatever. Now. The backing store of those cache can store only unique so if commit header and treenode root share the same font, they will be the same object.

To be clear, what is "location" supposed to be? "difffont", "commitfont", "monospacefont", or "font"?

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@amaiorano
Copy link
Contributor Author

Right. DiffFont, CommitFont.....

Okay, so this is what I understood before, and as I said before, this will be slower than what I presented here since we will pay for the longer dictionary lookups (comparing structs is slower than enum values). Also, upon changing a single font, I can't clear the entire dictionary; instead, I'll have to clear only the entries who's location is the current font being changed.

If you really want this, I can make this change.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email •
edited

@amaiorano
Copy link
Contributor Author

Do you see how keying on the style would mean you would only have possible keys per unique style value? So only one regular don't. Only one bold font. Only one bold italic font. See the issue?

/facepalm! Okay, I see now what you mean. I wasn't thinking about the combinations of styles! Okay, okay, that makes a lot of sense. Sorry for the confusion. I'll go ahead with your suggestion. Thanks.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018

If you want an enum of locations then that could work too. My point is you need to not key it just to the style. We have multiple fonts per style being used.

@amaiorano
Copy link
Contributor Author

If you want an enum of locations then that could work too. My point is you need to not key it just to the style. We have multiple fonts per style being used.

Yes. But just to be clear, my code was to get only variants of Application.Font with different styles, not the other types of fonts by style.

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@vbjay
Copy link
Contributor

vbjay commented Nov 3, 2018 via email

@amaiorano
Copy link
Contributor Author

Sounds like a good idea, but it's outside the scope of this change. My goal was to fix the problem of nodes being incorrectly bolded, which came as a result of cd68a51. Would you accept this change as is for now, and perhaps you can work on improving the Font handling as per your idea?

gerhardol
gerhardol requested changes Nov 3, 2018
View reviewed changes
Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

This PR reverses something that was "fixed" in #3795
It is possible to set not just the font but style too.
I would not mind if that PR was reversed, but the font selection should be reimplemented if so.
@RussKie took a decision, I suggest we stick with that.

image

Of course, if you for some exotic reason like described in #3795 selects another font, the bold (and italics) override can be weird, but that you will have to live with.

So likely should FontStyle.Regular be replaced with a custom type with Default or use null.

It feels incomplete to only use a cache for the application font and not the other fonts, even if not used there (right now, I have not checked though).
What would the performance impact be to not use the cache after this change?
What about a correction where the active branch is reset when closing (or when opening, if style differs)?

@amaiorano
Copy link
Contributor Author

@gerhardol Ah, I didn't realize that the AppSettings.*Font included style, but that makes sense. I will rework my change to be simpler and address only the bug here, even if it's not as efficient.

@amaiorano
Fix RemoteObjectTree branch folder nodes sometimes becoming bold
f39bfa6 
This bug was introduced in cd68a51 where setting TreeViewNode.NodeFont = AppSettings.Font was removed as an optimization. Problem is that for performance reasons, nodes are recycled, and it's possible for a node that was a LocalBranchNode for the active branch (thus, with bold font) to become a BranchPathNode. This would leave the BranchPathNode with bold font instead of the regular font.

This change brings back the former change, except this time we only update the node's font if required.
@amaiorano amaiorano force-pushed the amaiorano/fix-rot-branchpath-font-bolding branch from 96905c3 to f39bfa6 Compare November 3, 2018 23:33
@amaiorano
Copy link
Contributor Author

I rewrote the commit. I still kept the idea of setting the style for the nodes, but it's localized to the node code. This doesn't break #3795 as you pointed out. If you choose a bold font as AppSettings.Font, you won't be able to distinguish the active branch, but as you said, @gerhardol, this is something we'll have to live with for now (maybe in the future, we'll be able to expose more font settings, including which fonts to use for the ROT nodes).

@gerhardol gerhardol added area: left-panel Issues related to the Left Panel type: regression regression, normally to latest official release and removed 👓 status: needs review labels Nov 4, 2018
@gerhardol gerhardol added this to the 3.00-beta2 milestone Nov 4, 2018
@RussKie
Copy link
Member

RussKie commented Nov 4, 2018

It is possible to set not just the font but style too.
I would not mind if that PR was reversed, but the font selection should be reimplemented if so.
@RussKie took a decision, I suggest we stick with that.

If a user choses to use bold/italic fonts, they should be free to do so. The fileviewer control can't be easily customised, so #3795 ended up being half baked.
We can certainly consider rolling it back, but it should be done as a separate discussion/PR.

@RussKie
Copy link
Member

RussKie commented Nov 4, 2018

I'll throw a spanner in the works.
Do we really need to show the currently checkout branch in bold? Or we can present it in some other manner, that does not add as much complexity? For example, would it work, if instead of rendering the branch name in bold we would show a different icon?

@gerhardol
Copy link
Member

Do we really need to show the currently checkout branch in bold?

I believe bold/italics is a good way to mark differences.
Bold is used in a few other situations like for the subject of the checked out revision and for commits by the currently selected revision.

This should be included in 3.0, even if not in rc1/beta2.

@RussKie
Copy link
Member

RussKie commented Nov 4, 2018

...and for commits by the currently selected revision.

I plan to re-instate the highlighting before the official release, I'm struggling without it

@RussKie RussKie modified the milestones: 3.00-RC1, 3.00 Nov 4, 2018
@amaiorano
Copy link
Contributor Author

I agree that using bold is nice here, even if it doesn't work with the default font being set to bold.

So are we good with this PR? I know it looks a bit complicated in the SetNodeFont, but I think eventually if there's work done to improve font handling overall, this would eventually disappear (see discussion above).

@gerhardol
Copy link
Member

So are we good with this PR?

For me it is, but @RussKie decides and he has been working hard to get 3.0 RC1 out of the door and he has not decided yet.

@amaiorano I appreciate your contributions. Sorry that I cannot dedicate more time, busy in real life and I need to look at the 3.0 too.
If the 3.0RC1 is a success, development should be more normal again.

@RussKie
Copy link
Member

RussKie commented Nov 4, 2018 via email

@amaiorano
Copy link
Contributor Author

@amaiorano https://github.com/amaiorano - I too appreciate your contribution, but this won't be merged until we release v3. Right now we're in a "freeze" period, where only critical bug fixes get merged in. I know it feels disappointing, but we can't risk delaying the release. Please hang tight.

Oh no, it's fine, I understand needing to lock down to make the next release as stable as possible. I have no problem with any of my changes coming in afterwards.

@RussKie
Copy link
Member

RussKie commented Nov 6, 2018

Actually I've this is a significant issue, so it will need to be fixed before v3. I stand corrected.

RussKie
RussKie reviewed Nov 6, 2018
View reviewed changes
GitUI/BranchTreePanel/RepoObjectsTree.Nodes.cs
TreeViewNode.NodeFont = new Font(AppSettings.Font, style);
}
}
}
Copy link
Member

@RussKie RussKie Nov 6, 2018 •
edited

Choose a reason for hiding this comment

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

Can we simplify it as follows?

            protected void SetNodeFont(FontStyle style)
            {
                if (style != FontStyle.Regular && TreeViewNode.NodeFont.Style.HasFlag(style))
                {
                    return;
                }

                TreeViewNode.NodeFont.Dispose();
                TreeViewNode.NodeFont = new Font(AppSettings.Font, style);
            }

Any reason a node font would be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely want the NodeFont to be null by default so that it uses the font that was set on the NativeTreeView. This is the 99% case (or whatever), and in that case, we avoid allocating/disposing of node fonts. In the end, we only pay to dispose/allocate for the bold nodes. It's this optimization of making the nodes null for regular style that makes this code more complex, unfortunately.

It can be made a lot more simple at this level if we work on doing proper font management at the AppSettings level, like I did in the initial commit for this PR. But my change was flawed, and as per the long discussion with @vbjay, a system where we'd cache font + style + context with callbacks for when fonts change would probably be the way to go. For now, though, the change I provide here is optimal, behaves correctly, at the cost of a little complexity in the one function (SetNodeFont).

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the explanation. Makes sense.

@RussKie RussKie merged commit b37c234 into gitextensions:master Nov 7, 2018
@ghost ghost removed the ❕ priority: high label Nov 7, 2018
@amaiorano amaiorano deleted the amaiorano/fix-rot-branchpath-font-bolding branch November 7, 2018 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie left review comments

@gerhardol gerhardol gerhardol approved these changes

Assignees
No one assigned
Labels
area: left-panel Issues related to the Left Panel type: regression regression, normally to latest official release
Projects
None yet
Milestone
3.00-RC2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@amaiorano @vbjay @RussKie @gerhardol