[Sugar-devel] Patch review request for ticket #2290

[Sascha Silbe]

Excerpts from Dipankar Patro's message of Mon Sep 06 19:04:04 +0200 2010:

> I have attached the revised patch. (uploaded at bugs.sl.o too)

Please use git send-email to send patches so they appear as inline text
rather than attachments. With many email clients it's hard to reply to
text attachments.

I still think class TimeoutServerProxy could be eliminated and that
this change would make the code simpler and easier to understand.

> * changed things (subject, description, etc) according to Sascha Silbe's
> suggestions.
Thanks!

Let me suggest some further improvements:

> Subject: [PATCH] Register widget click event reflects a timeout on unavailability of servers

"Time out on registration process to prevent indefinite UI hang (SL#2289)"

> [Ticket #2289]

The convention so far was to append the ticket number to the subject so
it appears in one-line commit logs. Mentioning it separately is OK, but
if you're already changing the summary and description it makes sense
to adjust it.

> The register widget when clicked, used to cause sugar to freeze.

> Added a timeout facilitated ServerProxy() so that connection to schoolserver
> is dropped in case it is not available.

"Registration with the school server is currently done synchronously.
To prevent the UI from hanging indefinitely if the school server is reachable
but unresponsive we add an explicit timeout."

As you can see it's still not perfect, but hopefully conveys the
rationale behind the patch and the high-level changes it makes better.
The focus in patch descriptions is on what the effect of the change is
and why it is done, not so much how it is achieved (that's visible in
the patch itself).

Choosing a good description is hard, even after years of training. But
it makes life a lot easier if you're trying to understand some piece of
code months or years later.

Give "git blame <some file>" and "git log <commit ID>^..<commit ID>" a
try sometime to see how to use the commit log to understand some code.
(There are probably even GUI tools for that, but as I'm a console freak
myself I don't know about any).

> @ Marco : The default timeout is way too long (unable to find out exact
> time). Yes, the process is synchronous, thats why Sugar is freezing.

The chosen timeout of 10 seconds seems sensible to me, FWIW. It's long
enough to allow a busy server (even a remote one) to answer and short
enough so the user doesn't give up and reset the machine.

Sascha

--
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100906/a0fbb73d/attachment.pgp