Created an attachment (id=561) [details]
Application example to show the weird behaviour.

You could try with this simple program. You can select, with the buttons, if
the application will last on respond or not.

Created an attachment (id=562) [details]
Patch to take key press and release events timestamp into account when dealing
with short/long ESC keypresses

Comment 3 Xan López 2007-10-15 14:16:09 UTC

Hey, thanks for the patch. Just a few comments:

- In the future could you use -p for the diff command? It makes reading patches
easier, as it gives more context.

+                /* Added with lower priority to avoid queued key
+                   release events being proccessed later than the
+                   timeout */
+                priv->escape_timeout = g_timeout_add_full
+                  ((G_PRIORITY_DEFAULT + 1),
+                   HILDON_WINDOW_LONG_PRESS_TIME,
+                   hildon_window_escape_timeout,
+                   widget,
+                   NULL);

This looks like a good idea to me, and I think that we should apply it
regardless of what we do with the rest of the patch. The parenthesis around
G_PRIORITY_DEFAULT + 1 is not needed though :)

+                if ((event->time - priv->escape_press_time) >=
+                    HILDON_WINDOW_LONG_PRESS_TIME)
+                  {
+                    hildon_window_fake_close_event (widget);
+                  }
+                priv->escape_press_time = 0;

This on the other hand, I'm not so sure. It is a hack, and I can imagine
situations where it could break (like getting a ButtonRelease without a
ButtonPress. Yes, it can happen). Shouldn't lowering the priority of the
timeout  be enough to prevent it from running before queued events that got
stuck because the application is blocking the main loop?

I'll add Tommi in CC so he can comment about this.

(in reply to comment #3)
> - In the future could you use -p for the diff command? It makes reading patches
> easier, as it gives more context.

Thanks for the tip!

> This on the other hand, I'm not so sure. It is a hack, and I can imagine
> situations where it could break (like getting a ButtonRelease without a
> ButtonPress. Yes, it can happen). Shouldn't lowering the priority of the
> timeout  be enough to prevent it from running before queued events that got
> stuck because the application is blocking the main loop?

You are right, this is a hack but this is not new code. It has been
refactored from the hildon_window_escape_timeout function in order to
make use of it more easily.

About those situations, they are already handled, take a look to the
hildon_window_focus_out_event function.

Finally, I reckon it is not enough just lowering the priority since
you have to handle with key release event been handled before timeout
and having pressed the key for more than the set timeout.

I think the patch is safe in the sense that it is not adding any
strange hack that it wasn't already there.

Comment 5 Tommi Komulainen 2007-10-16 11:26:54 UTC

(From update of attachment 562 [details])
Looks OK to me. Though the ChangeLog should describe the problem and the why a
bit more than the how which can be seen from the patch.


>+                /* Added with lower priority to avoid queued key
>+                   release events being proccessed later than the
>+                   timeout */
>+                priv->escape_timeout = g_timeout_add_full
>+                  ((G_PRIORITY_DEFAULT + 1),

You should use GDK_PRIORITY_EVENTS + 1 there.


>+                if ((event->time - priv->escape_press_time) >=
>+                    HILDON_WINDOW_LONG_PRESS_TIME)
>+                  {
>+                    hildon_window_fake_close_event (widget);
>+                  }

There should be a comment there as it's not immediately obvious why you might
want to close the window on release. After all, there's a timeout used for
closing.


>       g_source_remove (priv->escape_timeout);
>       priv->escape_timeout = 0;
>+      priv->escape_press_time = 0;

Unsetting escape_press_time seems superfluous as its handling is always
preceded with check for priv->escape_timeout. No strong feelings, but if you
prefer unsetting the value you should do it consistently, also in
hildon_window_escape_timeout()

Comment 6 Xan López 2007-10-16 12:09:52 UTC

(In reply to comment #4)
> You are right, this is a hack but this is not new code. It has been
> refactored from the hildon_window_escape_timeout function in order to
> make use of it more easily.
> 
> About those situations, they are already handled, take a look to the
> hildon_window_focus_out_event function.

Ah, right, had forgot about that. The thing is quite ugly still, but as you say
it's the way things are. I suppose we can't do much better given how GTK+ works
and our desire of supporting misbehaving applications. If you can attach a new
patch with all the comments addressed we'll commit it ASAP.

Created an attachment (id=570) [details]
Patch to take key press and release events timestamp into account when dealing
with short/long ESC keypresses

I've modified the patch as suggested.

* More verbose Changelog.
* G_PRIORITY_DEFAULT replaced by GDK_PRIORITY_EVENTS (and parethesis removed).
* Comments in hildon_window_key_release_event.
* Resetting priv->escape_press_time variable in hildon_window_escape_timeout to
keep consistency.

Comment 8 Andre Klapper 2008-06-06 17:17:13 UTC

reassigning MDK's bugs to Rodrigo.

Comment 9 Andre Klapper 2008-07-18 17:12:41 UTC

Andrés: Does that patch still apply for Diablo? If not, can you please update
it for Diablo so we can try to get this in? Thanks in advance!

Andre, I'm not sure if I've understand your request.

In any case, it applies with minor offsets to the current SVN Head and to the
source package of the one currently in Diablo. The patch doesn't apply to the
ChangeLog file but I reckon you should change it in any case when applying it
:)

It also compiles cleanly.

Please, don't hesitate to ask anything you need.

Comment 11 Andre Klapper 2008-08-04 16:34:35 UTC

(In reply to comment #10)
> Andre, I'm not sure if I've understand your request.

In general, my request is to have a valid patch that can be applied to the
current Diablo codebase, so we can get this in and close this report.
Just trying to clean up the patch database in maemo Bugzilla. ;-)

Comment 12 Andre Klapper 2008-10-07 16:29:26 UTC

@Andrés: Can we get an updated patch? It's not useful to have it rotting here.

On the other hand, I expect some changes in hildon for Fremantle so I don't
know if it will be useful at all.

@Claudio, can you take a quick look at this? I highly assume it's WONTFIX for
Diablo, but if it's not INVALID for Fremantle we should consider this.

Comment 13 Andre Klapper 2008-11-22 19:50:15 UTC

@Claudio, can you take a look at this? I highly assume it's WONTFIX for
Diablo, but if it's not INVALID for Fremantle we could consider this.

Comment 14 Andre Klapper 2008-11-24 15:10:56 UTC

According to Claudio (desktop team) this is most probably still a valid issue
for Fremantle.

Comment 15 Neil MacLeod 2008-11-24 21:38:04 UTC

So let me get this straight: a perfectly good patch was supplied by the
"community" over a year ago, then ignored and never applied/released, and now
you're chasing the original patch provider to supply an updated patch despite
Nokia having done nothing with the original patch?

Is that an accurate reflection of what has happened here? If it is, can anyone
explain what went wrong? Where is the incentive for people to provide patches
when they are ignored? Some lessons to be learned, I hope...

Comment 16 Andre Klapper 2008-11-24 21:42:40 UTC

(In reply to comment #15)
> So let me get this straight: a perfectly good patch was supplied by the
> "community" over a year ago, then ignored and never applied/released

I wouldn't call Igalia developers "pure community", but quite often
subcontractors. ;-)

Comment 17 Neil MacLeod 2008-11-24 22:06:02 UTC

(In reply to comment #16)
> (In reply to comment #15)
> > So let me get this straight: a perfectly good patch was supplied by the
> > "community" over a year ago, then ignored and never applied/released
> I wouldn't call Igalia developers "pure community", but quite often
> subcontractors. ;-)

Hi Andre - thanks for the clarification however the question remains, why was
the patch ignored rather than applied? It's frustrating to see bugs hang around
for so long, and even more frustrating when a patch has been provided and then
reviewed positively and the bug is STILL not fixed!!

People move on, and expecting Andrés to provide an update to a patch he
provided almost 12 months previous and also accusing him of leaving it to rot
is perhaps a little unreasonable (not to mention even rude) when it is Nokia
that have failed to apply the patch in a reasonable amount of time - it is
Nokia who have left it to rot, not Andrés (who has probably moved on to another
project, and who could blame him?)

Is it any wonder some people believe that Nokia suffers from "Not invented
here" syndrome? I suspect that isn't always the case, rather the problem is
most likely due to poor internal communication, processes and a general lack of
involvement in the public Bugzilla ie. someone didn't pick this up that should
have done despite various comments from other Nokians (not that I'm blaming
them directly).

Let's hope this doesn't happen again. Earth to Claudio...

Whining is not something I consider relevant when deciding the priority I give
to my tasks, so you can either silently wait or provide technical insight on
the patch/bug. Discussions on Nokia relation with the community should go
somewhere else than bugzilla. Thanks.

Comment 19 Andre Klapper 2008-11-24 22:30:46 UTC

I think Neil knows about the correct places to discuss this...

> and even more frustrating when a patch has been provided and then
> reviewed positively and the bug is STILL not fixed!!

Provide an example - I didn't see feedback here for two months, so it sometimes
happens that not all bugs get handled they way they should. Reality bites,
especially if you have to clean up ~1300 existing bugs. But you know all that,
Neil...
Off topic, end of discussion here.

Comment 20 Andre Klapper 2008-11-25 13:40:42 UTC

WONTFIX for Diablo (not important enough and limited ressources to handle),
kind of INVALID for Fremantle (as some of the implementation stuff will change
here so it does not apply in this way).

I hope that in the future, Maemo Reconstructed could pick up such "WONTFIX but
valid" patches, but that's another issue and definitely again offtopic here...

Marking patches of interest to Diablo (Maemo4) community updates, please excuse
the noise.