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

Decoupled CompletionList and MethodCallTip from Scintilla. #1026

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

Neverbirth
Copy link
Contributor

  • This includes removing the controls from the MainForm, so they can be used in floating panels, dialogs, etc.
  • Removed Backspace from being a shortcut, it didn't make any sense.
  • Several UX improvements.
  • ScintillaControl a bit more similar to newest (old) ScintillaNet control.
  • This also changes the behaviour of shortcuts like Ctrl+Space to only work if the editor is focused. Plus backspace could also trigger some special functionality when in other panels.

There is always room for improvement in some places, but maybe not worth it at the moment. And as said back in the day, custom scrollbars are lost for the moment. More details in the complete old PR: #1020

- This includes removing the controls from the MainForm, so they can be used in floating panels, dialogs, etc.
- Removed Backspace from being a shortcut, it didn't make any sense.
- Several UX improvements.
- ScintillaControl a bit more similar to newest (old) ScintillaNet control.
@elsassph
Copy link
Member

@Meychi can you give it a try on crossover?

@elsassph
Copy link
Member

I'm going to give it a try asap to code at work and then merge if nothing explodes.

@elsassph
Copy link
Member

Trying the PR and I still get the custom scrollbars.
Great job BTW, having the list finally following the scrolling is really cool.

@Neverbirth
Copy link
Contributor Author

Yes, the custom scrollbars are there, but are over scintilla, so it's not working 100% as it should, because the caret can go below the scrollbars and the user cannot see what is in the current line.

@elsassph
Copy link
Member

Ow this is awkward - can't you reserve the space for the scrollbars?

@Neverbirth
Copy link
Contributor Author

Well, it's "not" possible, the custom scrollbars are normal controls, and therefore have to be inside the client area to be visible (system scrollbars are outside the client area), but since we are inside the control itself, the client area has to fill the whole space.

Some ways around it (and for any other control):

  1. Not adding the scrollbars to the control itself. The "simple" way (not so simple on escenarios with complex layout schemes), I don't like this method myself, plus maybe we'd have to wrap the controls inside other control, and bubble a lot of properties, methods, events...
  2. Modify the client area, and draw the scrollbars ourselves over the non client area. This one looks like a nice option, good for every control, but requires a lot of interop (really, a lot).
  3. Leave the scrollbars over the client area, and catch when we are going to run below them, and scroll ourselves. A lot of programs nowadays behave like this, with semi transparent scrollbars or the like, I don't know if it may be too tricky to implement, but for several controls we may not even need interop at all. I don't know for Scintilla how this would behave.

@elsassph
Copy link
Member

For 3. there's a built-in option:

sci.SetYCaretPolicy((Int32)(CaretPolicy.Slop | CaretPolicy.Even | CaretPolicy.Strict), 1);

@wise0704
Copy link
Contributor

wise0704 commented Feb 1, 2016

@elsassph That is one solution assuming that the height of the vertical scroll bar (17px) is approx. the height of one line.
Problems are that firstly the current y-scroll policy (Jumps|Even) is lost, and secondly with small fonts the text still hides below the scroll bar (second problem could be fixed by changing the slop value according to the text size however).

@Meychi
Copy link
Member

Meychi commented Feb 1, 2016

Does this present a breakage in the PluginCore? I tried to look but im not 100% sure.

@Neverbirth
Copy link
Contributor Author

It should be backwards compatible, if that's what you mean. But I really wonder if there are many plugins using the affected part, I only know of one, and it isn't using it directly.

@Meychi
Copy link
Member

Meychi commented Feb 1, 2016

By previous experiences we should expect the worst.

# Conflicts:
#	PluginCore/PluginCore/Controls/CompletionList.cs
#	PluginCore/PluginCore/Controls/UITools.cs
#	PluginCore/ScintillaNet/ScintillaControl.cs
@Neverbirth
Copy link
Contributor Author

Conflicts fixed yesterday.

@Meychi
Copy link
Member

Meychi commented Feb 3, 2016

On a quick test i noticed that the used font in the hint is incorrect. Also you removed Focus event which could lead to problems if a plugin hooked to that event. What about the scrollbar issue?

Otherwise everything seems ok.

@Neverbirth
Copy link
Contributor Author

The font issue is already fixed by Philippe in his local repository, so I didn't look into it. The focus event logic is removed, because now it's handled by .NET itself in the standard way as any other user control. The scrollbar issue I'm not fully sure, I know Philippe and Daniel looked a bit into it but I cannot tell how is it, I'd say Scintilla offers enough control and information to do it.

EDIT: Yes, you are right that if a plugin uses the old Focus event it would break, but I didn't see any (compatible with FD5 itself, for FD4 I saw some) and I'd prefer to remove the code, but it could be possible to add it rerouting it to the native .NET event.

@Meychi
Copy link
Member

Meychi commented Feb 3, 2016

Ok, so Philippe finishes this?

@Neverbirth
Copy link
Contributor Author

Oh, in Win 8+ or high dpi (I don't know exactly which one, or if both of them, also I don't know if it happens already with this branch, or only with the added changes by Philippe) there is a problem that pop ups randomly with the call tip. I'm working on something related, so I don't know if he prefers to wait for it.

@Meychi
Copy link
Member

Meychi commented Feb 3, 2016

This needs to be rock solid and not introduce any compabity issues before merge. I will now test this and current dev in CrossOver.

@Neverbirth
Copy link
Contributor Author

So what happened with CrossOver? I couldn't find any problem on Win7 so far (this was done near a year ago, and was tested for months back then, but I've been using it again just in case there was some problem because of some newer commits). Would be nice to see it already merged as is, the rest of the problems are being worked on, and it will allow me to make some new PRs.

Conflicts:
	PluginCore/PluginCore/Controls/RichToolTip.cs
	PluginCore/ScintillaNet/ScintillaControl.cs
…vior, and it's how it works in other IDEs. I prefer the new way, because it allows to scroll the document when over some control without wheel, but it has problems with the window list, I guess because wheel support is not native in there.
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