Created attachment 277483 [details]
Scrolling Mockup

Here's a mockup that tries to implement GMail-like scrolling.  Basically, the <div> over which the widget will be placed is made big enough to contain the widget without scrolling.  However, when the widget is actually allotted space, it is only given the visible space of that <div> (or a minimum value).  This means the widget's top and bottom lock onto the top and bottom of the viewport as the <div> scrolls past behind it.  Then, we adjust the vertical adjustment of the inner WebView to the difference between the top of the widget and the top of the <div>.  This makes the inner content appear to track the outer content during scrolling.  We also hook things up the opposite way -- when the inner content scrolls, the outer WebView scrolls by and equivalent amount.  This ensures that when the inner view is scrolled to keep the cursor in view, the outer view moves with it.

The problems I know of:
1) I use a fixed minimum height for the overlaid widget.  With the composer, we should probably adjust it as the state changes, so that the inner WebView has a fixed minimum.
2) I'm assuming Gdk pixels and WebKit pixels are the same.  On high-res screens, they may not be.
3) Mouse wheel scrolling isn't perfect.  It works better than it has any right to, as the scroll events take several different routes to their destination.  What I want to do is to keep the overlaid widgets from receiving any scroll events, and instead have all scroll events go to the outer WebView.  I haven't figured out how to make this work, though.

*** Bug 730906 has been marked as a duplicate of this bug. ***

Very cool!  This looks much like the way Gmail deals with this problem.  I did notice that the mouse wheel events don't get handled if the cursor floats over the "this is top" / "this is bottom" markers; I suspect this is what you're talking about in (3).

I'm not sure about (2).  I do know that a hi-res patch landed in GDK/GTK at the same time it landed in WebKitGTK, so perhaps that will alleviate the problem.

But this proof-of-concept certainly goes a long way toward fixing the problem.  It looks like the idea here is to never have a scrolling region within the embedded WebView.  That's great, but it will be a problem as long as we expand quoted text in replies.  If we could collapse it by default, only expanding it when the user requests it, that would really make this work.  At that point we might be stuck and have to use a scrolling region (I've seen quoted text in some conversations grow to ginormous lengths).

Regarding scroll events, I don't have a good answer for you -- I know there are parameters with GtkEventBox that deal with something similar; it might be a starting point toward a solution.

I've been playing with the scrolling issue, without a whole lot of luck.  The best solution I've been able to come up with is to attach scroll-event handlers, which re-route the event to the WebView, to *every single widget* in the overlay.  This seems rather silly, but with GTK's bubble-up approach, I haven't been able to find a better one.

For the issue of long bits of quoted text: I think a way to compress it could be useful, although this may depend on the top- or bottom-posting issue.  (Someone bottom posting probably wants the text expanded, in order to place the replies inline.)  But I don't think that blocks this solution.  Note that the label at the bottom of the embedded widget (equivalent to the send button) remains on screen for most of the scrolling of the widget.  You don't need to scroll down to the bottom to send the email.  The only time when this becomes problematic is when the composer is in the middle of a conversation, and you want to read messages following it.  But that's a relatively rare use case, so I don't think we should delay this until that's worked out.

So, should I start implementing this for real, or do you have something else we should play with first?

It is possible to recursively iterate over every widget in a GTK container, so that would at least make it a little easier attaching event handlers without missing something later that's added.  There's probably some issues that way too, though (some widgets use private internal widgets as containers), but mentioning it if that will help.

The quoted text issue is not vital, just something for us to keep in mind.  I hadn't thought about how that would work with bottom-quoting.

I think it would be great to implement this for real.  I have no other approaches in mind but from using the inline composer so far, I'm already eager to see this land.  The only other inline composer nit I'd like to attack is to avoid the dialog box query when switching away from a draft (i.e. always present drafts in an inline composer).

Created attachment 278281 [details] [review]
New scrolling system for inline composers

The editor in the composer no longer shows its own scrollbar.  Instead,
the conversation view allocates enough space to hold the composer
without any scrolling.  The composer then positions and scrolls itself
to create the illusion that the outer scroll bar controls it.

WIP -- the editor doesn't shrink properly when its content shrinks.

I'm using the vadjustment of the webview in the composer to tell what the desired height of the webview is, but that only works when that desired height is greater than the allotted height.  I then increase it to the desired height, so we don't notice when the desired height shrinks.

Yes!  Very cool.  This feels a lot better than how it works now and I love that the top and bottom bars remain in view while scrolling.  I'm sure getting this all wired in was a trick, but it doesn't look like, in the final assessment, a lot of code was necessary.

I'm not certain if I can offer any good advice about the vadjustment problem.  The whole GtkScrolledWindow <-> GtkViewport <-> WebView relationship is so tricky.  (I recall us battling a problem there a few weeks ago.)  Have you tried watching the WebView's allocation itself, i.e. the size it reports it needs?

Are there any other problems you're seeing with this?  For me, with just a little use, it looks real close.  I'm going to tentatively mark this for 0.8.

(In reply to comment #8)
> Have you
> tried watching the WebView's allocation itself, i.e. the size it reports it
> needs?

That's not going to help, I'm afraid.  The problem is that the WebView has been allocated more room that it really needs.  It says, okay, fine, I'll just put some blank space at the bottom.  But it looks like there's no way to tell, in GTK at least, that it has some extra room.  I fear that the solution is to put everything in a div and then measure the size of that.  It's an extra trip into the DOM, but I'm not seeing a way around this.
> 
> Are there any other problems you're seeing with this?  For me, with just a
> little use, it looks real close.  I'm going to tentatively mark this for 0.8.

I'm also seeing an issue that the editor isn't getting scrolled all the way to the bottom when it ought to be.  Most often, I see this when scrolling with the mouse wheel.  As the bottom of the composer moves across the screen, sometimes the editor jumps a little.  I fear the issue here is that there are too many signals being passed around, so something's getting confused.  Scrolling the outer webview causes the overlaid widget to be resized, which causes the inner adjustment to change, which makes us reconsider the size of the inner webview, which could cause us to change the size of the DOM element (though it shouldn't), which will trigger another scroll adjustment, which....  I'm a bit scared to trace it through carefully, since I could discover that it can't work, at which point it'll obviously stop working.  But maybe I need to dig in and add some state flags to avoid too many layout cycles.

From the crazy-like-a-fox-or-just-plain-insane? department:  Instead of making the editor a second WebView overlaid on the first, we could make a div in the conversation WebView editable.  The top and bottom of the composer could still be overlaid as separate widgets.  This would solve all the scrolling and resizing issues, since it'd all be handled by the single DOM.

This would open several other cans of worms: All the Composer WebView stuff would have to be duplicated in the conversation WebView, but only active in appropriate divs.  Detaching the composer would be more complicated than just reparenting the widget.  Getting the right keyboard shortcuts active would likely be a pain.  All in all, it's probably not a good idea.  I'm just trying to toss out some lateral thinking.

Boy, this would stretch GtkOverlay's utility to the point of insanity.  I get the idea and see where you're going with it, but I really do think we landed on the right solution with a single overlaid widget.

I'm not nearly as close to the resize problem as you are, but here's an idea: what if the edited text was mirrored to the background div?  That is, the text is inserted to the DIV's inner text and the DIV's new size is used to resize the edit window?  I suspect this is too-good-to-be-true, but lateral thinking and all.

Created attachment 281447 [details] [review]
New scrolling system for inline composers

I've been slowly picking at this, and I realized I should update you on 
the current status.  The mirrored div is interesting, but it requires a 
awful lot of DOM actions for each keystroke, and they tend to be 
expensive.  If we're going to be messing about with the DOM, we might as 
well just measure the thing in the editor.

Unfortunately, if you measure the size of the body element, you get the 
same problem we got with the adjustments -- it won't shrink back down if 
it doesn't need the space.  To get around this, I put a div in the body, 
make that editable, make the webview non-editable, and then measure the 
size of that div to work out the space needed.

There are two problems with that that I'm aware of.  First, there's no 
general "the text has been modified" signal, so instead we have to watch 
for all things that might modify the text.  Right now, we're only 
looking for key strokes, but in the future we'll need to catch cut and 
paste events, as well as other things I haven't thought about yet.

Second, we have to make sure all the resizing of HTML elements and GTK 
elements and the setting of scroll adjustments all happen harmoniously.  
To that end, there are several things put off to idle handlers, as well 
as a flag about scrolling state.  It's still not working correctly when 
first opened (do something to change the height of the composer to get 
the correct display), but I think it's working most everywhere else.  
But it's already complex enough that I'm forced to attack bugs 
empirically, rather than by reason.

There's still some work to do, but I thought someone else should check 
out what I have and let me know if this is still going in the right 
direction.

Robert, I think this is quite promising!  It seems smooth to me and the patch looks clean.  I have a couple of thoughts on what you wrote above and the patch.

One annoying bug I'm seeing is with a new message (not an inline reply).  If I press and hold Enter a scrollbar appears on the editor and my signature scrolls out of view (i.e. the viewport doesn't follow the cursor).  If I scroll down so my sig is in view then hit Enter again, the viewport moves to the top again.  If I scroll down quickly but not press the keyboard, the viewport will move to the top when auto-save kicks in.  I suspect all this is related to a central problem.  I don't see it in master.

Regarding the signal for "text modified", I recall that we use (or used to use) an "undo available" property or signal for something like that.  Actually, whatever we use for the auto-save drafts might work for you.  Can you piggy-back on that?

I do see the second problem you describe, where there's some jerkiness the first time the editor expands but from then on it gets smoother.  I'm perfectly happy to ticket that for later.  If you're close or have some leads, I would say let's try and iron it out, but if it looks hairy, it's liveable.

Regarding the use of the idle handler, it's a fact of life in GTK land, especially with allocation issues.  I will point out that Idle.add() defaults to Priority.DEFAULT_IDLE which is *lower* than GTK's resizing (HIGH_IDLE + 10) and painting (HIGH_IDLE + 20) operations.  You might trying using Idle.add_full() with a priority of HIGH_IDLE or DEFAULT and see if that helps any.

Finally, one coding nit regarding casting.  This is discussed more fully in our coding conventions under "Casting" (https://wiki.gnome.org/Apps/Geary/CodingConventions, I can't link directly to the section), but short story is, when you use the "as" operator, the returned variable should be nullable and null should be checked before using.  Most places in this patch, I think you want to use the C-style casting.

Created attachment 283630 [details] [review]
New scrolling system for inline composers

Finally forced myself to look at this again.  The scrolling bug was 
caused by the overlay child position call, which tweaks the adjustments 
to support our scrolling illusion.  It no longer does this for new 
compositions.

There is in fact a user_changed_contents signal that we use to set the 
draft timer.  Now we use that rather than the keypress signal to detect 
possible changes in height.

I found that if I call present() on the composer, it gets fully resized 
and scrolled into view.  The problem with this is that it often takes up 
the entire view, depending on how long the message you're replying to 
is.  But I think the solution here is to compress the quoted text when 
we open the composer (bug #730484).

I haven't played with any of the idle priorities.  The one problem I 
notice is that the inline composer often anticipates the scrolling with 
the scrollbar is dragged.  This isn't done in an idle, though, so I 
don't know if we can do anything about it.

Review of attachment 283630 [details] [review]:

We're reaching the land of code nits and little more:

* composer-embed.vala:9: Use a const instead of a static int.

* Should ComposerWidget.get_text() now merely be:
  return html_to_flowed_text(get_html());

And that's it, code-wise.

I do wish we had a handle on collapsing quotes in the editor (bug #730484).  A couple of times I've tried replying to a long email and it's a little annoying that the composer takes up the entire window.  I don't think there's anything else we can do about it without the collapsed quotes.  I did look at gtk_window_present() (which is really gtk_window_present_with_time(), see https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n9529) wondering if we could break down the call in our own code and get the resizing advantages but have more control over the scrolling.  The call is fairly simple; I don't think there's much we can do there.

I see this too: in a conversation I have three collapsed messages (not compressed) and a fourth short one that's open.  Altogether they take up about 60-70% of the viewport, no scrollbars.  If I reply the composer opens up enough to create scrollbars but I noticed that there's a substantial amount of empty space at the bottom of the composer.  When I start typing the window "jumps" and resizes.  The empty space at the bottom of the composer disappears, leading me to believe this should be the height it should've started with.  Are you aware of this?  I'll attach some screenshots to better show what I'm talking about.  I've seen this a couple of times as well, but inline the quotes, I'm (hoping) this can be fixed before landing the patch.

Otherwise, I have no issues with this and look forward to getting it in for 0.8.

Created attachment 283935 [details]
No composer

Note that all these shots are grabs from directly under the GtkHeaderBar to the bottom of the window; there's no viewport above the top of the images.

Created attachment 283936 [details]
Reply All button pressed (note empty space at bottom of composer)

Created attachment 283937 [details]
Key pressed (started typing reply)

html_to_flowed_text() takes a DOM.HTMLElement, not a string, so it needs to stay as is, I think.

I've tried changing ComposerEmbed.present() from using scroll_into_view(true) to scroll_into_view_if_needed(false), which should result in less scrolling.  It won't help with long quotes, though.

I was trying to avoid putting a present() call in the idle call.  There's already a present call in the constructor.  Since the reply hasn't been sized yet, this ends up putting 300px of the composer in view, which is a nice amount.  But then there are these other problems....

I just started noticing the same problem you did with short messages.  Actually, the same problem exists for long ones as well -- it's just less noticeable.  Try scrolling to the bottom, and you'll notice that the motion ends before the scroll bar reaches the bottom.  The problem is that the <div> is being made too tall, because the space allotted for the chrome is too large.  I assume this is because it hasn't yet made all the bits invisible for the current mode.  Somehow, we need to wait for the embedded composer to settle down and figure out its final size before we call the recalc_height() method.  Any idea for the signal to listen for?

(In reply to comment #19)
> html_to_flowed_text() takes a DOM.HTMLElement, not a string, so it needs to
> stay as is, I think.

Ah, I didn't catch that.

> Somehow, we need to wait for the embedded composer to settle
> down and figure out its final size before we call the recalc_height() method. 
> Any idea for the signal to listen for?

I'm not positive, but the first signal I'd try is "map".

Created attachment 284247 [details] [review]
New scrolling system for inline composers

I think I've gotten this now.  The system for setting visible widgets in 
ComposerWidget is a bit wonky -- show_all() is called on it, but we only 
want to show some widgets, depending on the state.  Before, we had been 
resetting visibilities after the show_all() via an Idle.add().  Then, we 
were trying to run the recalc_height code after the layout that resulted 
from this idle code.  This was causing this timing nightmare.

But we already had a custom show_all() handler!  So I just moved the 
visibility-setting code into that.  This cuts out the timing problems, 
and a simple Idle.add() in the ComposerEmbed is all we need to delay the 
recalc_height() enough.

I left the less-agressive ComposerEmbed.present() in there.  It's not 
actually making a difference in most cases, but I think it's a better 
way to go.  I also took the present() call out of the idle callback, 
which keeps the reply from taking up all the screen.  It's still a bit 
hacky, and we may want to revisit it when we get compressed quotes in 
replies working.

Review of attachment 284247 [details] [review]:

This might sound stupid, but I didn't realize from the prior patches (or the Idle handler) that show_all() was giving you this trouble.  Are you aware of Gtk.Widget.no_show_all?  Would that solve or ease this issue?

I'm good with this patch.  If you want to commit as-is, fantastic.  If you want to explore the no_show_all route, your call.

I didn't remembered the old idle handler when I was putting this 
together.  Had I done so, I would have realized the problem sooner.

I left things as is.  I tried to use no_show_all at some point in the 
past (not for this issue), and I never got it to work.  So now I'm a bit 
shy about it.  I don't think we want it here, anyway, since we're 
setting visibility based on state.  Doing that by hooking no_show_all to 
the state feels too indirect for my tastes.


Attachment 284247 [details] pushed as c36ff01 - New scrolling system for inline composers