Skip to content

optimize multiline entry #25

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

Open
wants to merge 10 commits into
base: GtkMultilineTextEntry140723
Choose a base branch
from

Conversation

sevoku
Copy link

@sevoku sevoku commented Jul 26, 2014

Hi,
your frame implementation is not very nice, since you draw the frame yourself. You should use a native frame to keep the native feeling and honor users theme settings.

@@ -272,9 +272,35 @@ internal static void Dispose (this Cairo.Context cr)
((IDisposable)cr).Dispose ();
}

public static void RenderPlaceholderText (Gtk.Widget widget, Gtk.ExposeEventArgs args, string placeHolderText, ref Pango.Layout layout)
Copy link
Author

Choose a reason for hiding this comment

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

your change breaks this feature for PasswordEntry and SearchTextEntry.
I've added specific extensions methods for Gtk.Entry and Gtk.TextView. RenderPlaceholderText should not be applicable on other Gtk widgets (Gtk.Widget) so lets make it private.

int width;
if (!MultiLine && lastHeight != allocation.Height) {
lastHeight = allocation.Height;
XLayout.GetPixelSize (out width, out lineHeight);

Choose a reason for hiding this comment

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

Placing a TextEntry and a PasswordEntry side-by-side shows that the PasswordEntry, still with the Gtk.Entry as backend, is a little taller than the TextEntry.

I sugest add 6 pixels to the lineHeight and set the PixelsAboveLines to 3 pixels... This way they will look alike... at least they did here on my pc (Xubuntu 14.04)

Copy link
Author

Choose a reason for hiding this comment

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

yes, but its a wrong way to set PixelsAboveLines here, cause it remains if MultiLine is changed after initialization. Lets set it together with the MultiLine prop.

@sevoku
Copy link
Author

sevoku commented Jul 27, 2014

Hhm, with this approach we have still inconsistent style compared to Gtk.Entry (and so PasswordEntry and SearchTextEntry). Many Gtk themes style Gtk.Entry differently. On Ubuntu Gnome 14.04 all Entries have rounded frame corners and the frame border has a different color, when the entry is focused.

I've tried a dirty approach by using a table (instead of a frame) and adding a Gtk.Entry behind Gtk.TextView (see http://stackoverflow.com/a/17977591). This solves the problem with rounded frame corners, but the Entry in the background needs a focus to draw the colored frame correctly, which steals the focus from TextView. So not really applicable.

@thiagojedi
Copy link

Is there any way to change the Backend widget on the fly?
I mean, with Multiline set false it uses Gtk.Entry and with true it uses
Gtk.TextView.

Both Backends implements the same interface, should be no problem
there.

@sevoku
Copy link
Author

sevoku commented Jul 27, 2014

I've done this already for me that way, but it is really dirty and the code is hard to read, because all methods have an if (MultiLine) .. else.

A better solution would be to remove the Xwt.TextEntry.MultiLine completely (or better mark it as Deprecated) and add a Xwt.TextEntryMultiline class, derived from TextEntry, but bind it to a new ITextEntryMultilineBackend interface. For Wpf and Mac TextEntryBckend would implement both (ITextEntryBackend and ITextEntryMultilineBackend). Gtk would register different backends (the old one for ITextEntry and the new one only for ITextEntryBckendMultiline).

This would be the cleanest solution, but a massive change of the Xwt API.

@thiagojedi
Copy link

I was thinking more in the way of a 2nd level backend.
We make the "TextEntry" class route the method calls to a
TextEntrySingleline or TextEntryMultiline, the same way the Xwt itself
does.

Most of the complexity will be on the Multiline property setter,
as some properties like the Placeholder and the Text will need to be transfered
from one object to the other.

It doesn't break the API, and it would be way cleaner than a lot of conditionals.

@sevoku
Copy link
Author

sevoku commented Jul 27, 2014

oh, yes, this would work of course. But I personally don't really like both ways of switching backends. It makes the backend code much more complex and makes it harder to implement backend specific code on the consumer part (since you have to check what backend is actually used).
I've implemented some little API changes and will push them tomorrow to my repository. The changes are not very deep and should have no regressions, since the old api is only marked as obsolete.

@lytico
Copy link
Owner

lytico commented Jul 29, 2014

Thanks for your inputs. I'll review it in the next days.

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.

3 participants