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.

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

Implement faded (configurable colour) zeroes in the Hex Editor #4098

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Asnivor
Copy link
Contributor

@Asnivor Asnivor commented Oct 25, 2024

This implements a HexLabelEx control that replaces the LocaLabelEx 'AddressesLabel' control in HexEditor.cs.
It overrides OnPaint(), parses the incoming string and draws the string out using different colours depending on whether the hex value is '00' or not.
A new colour selector is also implemented within HexColor.cs so the user can choose which colour they want displayed for 00 values.

This addresses #3769 and the duplicate #3930

Notes:

  1. I could not find a way to have all this stuff directly within HexEditor.cs itself. Maybe it's just a lack of knowledge, but I needed to intercept the OnPaint control method and NOT call base.OnPaint(). But if I disable base.OnPaint() in the control, the PaintEventHandler is never called. shrug.
  2. I'm only able to test this on Windows currently. The various arbtrary spacing floats used in HexLabelEx would have to be tested on linux and modified accordingly (there is already an OSTailoredCode.IsUnixHost check in the constructor ready to be added to).

I'm sure it can all be cleaned up, but I'm stuck at the moment on the 'how'.

@Asnivor Asnivor requested a review from YoshiRulz October 25, 2024 15:50
..if this is even a possibility
@Asnivor Asnivor requested a review from vadosnaprimer October 25, 2024 16:11
@Asnivor
Copy link
Contributor Author

Asnivor commented Oct 25, 2024

image

@Asnivor
Copy link
Contributor Author

Asnivor commented Oct 25, 2024

image

Copy link
Contributor

@vadosnaprimer vadosnaprimer left a comment

Choose a reason for hiding this comment

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

Approving the look, just add more info the PR title.

Copy link
Member

@YoshiRulz YoshiRulz left a comment

Choose a reason for hiding this comment

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

Wrong approach IMO.

Not sure if you considered this, but when implementing this feature, make sure it's as fast or faster than the current version. (I imagine there's enough leeway in the stale code to allow for being faster overall despite doing more work.)

@@ -20,6 +20,7 @@ private void HexColors_Form_Load(object sender, EventArgs e)
HexFreeze.BackColor = _hexEditor.Colors.Freeze;
HexFreezeHL.BackColor = _hexEditor.Colors.HighlightFreeze;
HexHighlight.BackColor = _hexEditor.Colors.Highlight;
Hex00.BackColor = _hexEditor.Colors.Foreground00;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use Zeroes in identifiers instead of 00

this.colorDialog1 = new System.Windows.Forms.ColorDialog();
this.label7 = new BizHawk.WinForms.Controls.LocLabelEx();
this.Hex00 = new System.Windows.Forms.Panel();
this.colorDialog1 = new System.Windows.Forms.ColorDialog();
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace noise can be cleaned up. If you want I can do it immediately before merge.

if (AddressesLabel.ZeroColor != Colors.Foreground00)
{
AddressesLabel.ZeroColor = Colors.Foreground00;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this belongs here

<ItemGroup>
<Compile Update="LabelEx\HexLabelEx.cs">
<SubType>Component</SubType>
</Compile>
Copy link
Member

Choose a reason for hiding this comment

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

Does this have any effect? If so then I'd expect the rest of the controls in this project to be missing their IDE icons or whatever, since they're obviously not listed here.


namespace BizHawk.WinForms.Controls
{
/// <inheritdoc cref="Docs.LabelOrLinkLabel"/>
Copy link
Member

Choose a reason for hiding this comment

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

Should add a <seealso/> to those docs

foreach (string line in lines)
{
// split left and right panes
string[] panes = line.Split('|');
Copy link
Member

Choose a reason for hiding this comment

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

Really don't like this. The caller has full knowledge of the content that's being passed in and should be able to make separate calls before or instead of passing in a concatenated line.

@YoshiRulz YoshiRulz changed the title HexEditor Implement faded (configurable colour) zeroes in the Hex Editor Oct 25, 2024
@Morilli
Copy link
Collaborator

Morilli commented Oct 26, 2024

This does not work when the data size isn't 1 byte.

@Asnivor
Copy link
Contributor Author

Asnivor commented Oct 26, 2024

Sounds like this needs a lot more work. It should probably integrate with the highlighting positioning code in the calling class. I'll work on it when I have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants