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

Improve handling for displaying readme controls #35

Closed
wants to merge 5 commits into from
Closed

Improve handling for displaying readme controls #35

wants to merge 5 commits into from

Conversation

Xanfre
Copy link
Contributor

@Xanfre Xanfre commented Aug 18, 2019

The readme controls are currently either made visible or hidden on the MouseMove event of the control. If the cursor is located over the readme box, they are shown, and if it is not, they will be hidden; however, if another window eclipses the form and the cursor is moved directly from the readme box to this window, the controls will remain visible. Having this functionality instead governed by the MouseEnter and MouseLeave events provides better behavior in this regard and intercepts fewer events. As an aside, there is currently a bug in which the form control box buttons will highlight if the mouse is moved near the top right corner of the readme box panel while the form is maximized. The message filter seems to be the culprit of this.

Additionally, FMsDGV has been modified to be made more visually appealing. The dark gray background of the grid is a rather jarring contrast to the appearance of the rest of the form; thus, a lighter background has been set to maintain the visual pattern, though I suppose this point is rather subjective. It has also been made impossible to have the individual columns appear "selected", as this should not be possible if the only way cells can be selected is by the entire row.

@FenPhoenix
Copy link
Owner

To the points:

  • Using the mouse events actually doesn't work like it seems to at first. It's the first thing I tried, and the reason I switched to the mouse hook is because it's not a single-control mouseover detect that's needed, but an area detect. The readme area can be in a state where the RichTextBox needs to be hidden but there still needs to be the readme controls showing (when the readme is HTML, the RichTextBox is hidden, a "View HTML readme" button is shown, and we still need the readme controls because there might be multiple readmes and we need to have the selection box up there). I also remember there being other cases where things didn't work right with the mouse events that weren't obvious at first, and unfortunately I don't remember what they were, but I definitely remember the mosue events came with way too many asterisks and how relieved I was to be able to use the nice mouse hook and skip the whole control hierarchy persnicketiness and just detect a region flat in the window.

  • I've been noticing the control box buttons highlighting bug myself recently but keep forgetting about it. It's a weird one, and I'll look into it.

  • I acknowledge that the readme controls stay visible when you mouse off the form, but it seems inconsequential, personally. In order for it to occur, you have to have another window partially overlapping and blocking the readme anyway, so having the top-right controls block a portion of it is the least of your problems then. Still, I'll see if I can maybe use a MouseLeave event in addition the hook to handle that. Of note is that I could use a global mouse hook to detect when the mouse moves out of the window entirely, but the global hook causes a problem where resizing the window becomes glacially laggy (which is random and bizarre). In fact, I'm starting to wonder if the mouse hook library I'm using is buggy. I know how to write my own p/invoke hook code and have just been lazy in using the library, so maybe I'll try that.

  • You're right that the datagridview background color is a heavy contrast. Personally I kinda like the sharp delineation between "something" and "nothing" (and being used to NDL which looks the same) but I guess it's neither here nor there... I wish more people would give me feedback so I would know whether it's a good idea to make a change like this. It might drive some people crazy. I dunno.

  • You're absolutely right that the column headers shouldn't highlight individually when the control is clearly set to a mode where there's no concept of individual columns. In fact it's a relatively new thing; you'll notice NDL doesn't have it, because it targets an earlier version of .NET Framework that hadn't yet introduced this. It's supposed to be an accessibility feature for better visibility of the current column, but as you can see it still does it even when we're in whole-row selection mode. I tried for quite a while to get rid of it, but it's controlled by an internal method that I can't even change with reflection. I can get rid of it by disabling a whole swath of accessibility features ("level 2") but who knows what genuinely useful stuff that might break. I see you fixed it by disabling visual styles, but I'm not keen on that solution. I'll tell you what though, .NET Core 3 is coming out very soon, and with it will come MIT-licensed WinForms source code. I'm planning to switch AL to .NET Core 3.1 (which is the "long-term service" release) when it comes out in November, and I'll be able to just grab the DGV code, modify it myself, and then we can keep visual styles but get rid of that godawful highlighting.

@FenPhoenix
Copy link
Owner

FenPhoenix commented Aug 18, 2019

Quick testing shows the control box flickering issue goes away when I return from the message filter before the WindowFromPoint() call:

var hWnd = InteropMisc.WindowFromPoint(pos);

That's truly strange, as that call shouldn't be affecting anything, but only returning a window handle. Well, anyway, the only reason I'm using the message filter in addition to the hook is that the hook library doesn't return the complete set of values from the p/invoke call (notably it's missing the "which keys are pressed" value so I can't tell if ctrl is pressed which I need). If I rewrite the mouse hook from scratch (fairly easy, there's actually a nearly-complete version somewhere back in the commit history), then I can handle this and then I can get rid of the message filter and maybe that'll fix it, who knows.

@Xanfre
Copy link
Contributor Author

Xanfre commented Aug 18, 2019

The issues addressed here are indeed inconsequential to a certain extent, though I believe addressing them would serve to supplement the user experience. Somehow, in my testing, I neglected FMs with HTML readmes. This is easy to account for, fortunately, and brief testing seems to show that using MouseLeave events to supplement the mouse hooks is a viable solution. I have not thoroughly explored the employed hooks library, but if they are the source of this odd control box highlighting issue, perhaps a rewrite would be preferable.

I agree that disabling visual styles in the FMs list is not a particularly elegant approach. Unfortunately, this is the only for such property changes to take effect, as they seem to be completely ignored in favor of the default style. Though if you do plan to migrate to .NET Core 3.1 soon, it would be most advantageous to wait and address this in a more direct manner.

If there is nothing more of use in this request, I will close it.

@FenPhoenix
Copy link
Owner

I fixed the control box highlighting issue. It was a dumb logic error on my part: I was trying to get an x and y position from the prefiltered message before I'd checked whether the message's value was in fact supposed to be a position, thus getting garbage data if it wasn't.

Moving onto the move-off-the-window problem next.

Thanks for reporting these things though, even if I may whine about them sometimes, it's really good that you're finding these visual glitches and corner cases because I might never find them by myself, so thanks. And even if I reject a PR we often get useful ideas from the discussion nonetheless.

@FenPhoenix
Copy link
Owner

FenPhoenix commented Aug 18, 2019

Your background color point reminds me again though of the idea of letting users customize the color scheme or theme to the extent practicable, some people were asking for a dark mode for instance. Maybe I should make the dgv backcolor part of such a feature. WinForms makes that a little trickier, but I should keep it in mind.

@FenPhoenix
Copy link
Owner

Fixed the readme mouse leave bug.

@Xanfre
Copy link
Contributor Author

Xanfre commented Aug 18, 2019

Excellent. All seems to work well, though including the return statement when intercepting the mouse leave call in the ComboBoxCustom class causes the box to remain highlighted after the mouse has left its boundaries, as the procedure that controls this is not executed.

The FMs list background color comes down to preference, I suppose. Providing an option may be welcome, provided other controls are recolored accordingly, as you say. Despite the fact that not all elements can be changed, perhaps a "dark mode" can be achieved to a certain extent.

@FenPhoenix
Copy link
Owner

You're right, my bad. Fixed.

As time has gone on I've gotten more and more used to doing my own work to make WinForms act the way I want. I'm pretty sure that scroll bars and tab controls can't be fully recolored by default, but I'm certain that with some custom work I could make it all happen. A custom tab control would be a good idea for other reasons too, and in fact I already essentially have one in the Settings form. I'll add this to my notes for .NET Core 3.

@Xanfre
Copy link
Contributor Author

Xanfre commented Aug 18, 2019

With that, it seems everything brought up here has been addressed. The rest should be facilitated by the general release of .NET Core 3.

Thank you for looking into these.

@Xanfre Xanfre closed this Aug 18, 2019
@FenPhoenix
Copy link
Owner

No prob, thanks for keeping me on my toes.

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.

None yet

2 participants