[haiku-development] Re: My SetViewUI ... patch
- From: looncraz <looncraz@xxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Mon, 5 Oct 2015 15:31:25 -0500
On 10/5/2015 09:20, Axel Dörfler wrote:
https://www.haiku-os.org/development/coding-guidelines
Done. Thought I had kept to them fairly well, and I generally did,
except in the DelayedMessage mess.
I cleaned that all up, hopefully acceptably.
- Wrong number of blank lines (for example between header and
includes, between methods, etc.). You can't do much wrong by using 2
blank lines outside of methods (not zero, not three), and one blank
line inside methods.
I found two or three places where this was true - you have a secret to
how you find these so easily?
- Bad naming of classes (DMMessageData)
I hated it when I named it ;-) Now DelayedMessageData.
No idea of a better name for the scheduled message. I could hand over
ownership of a message and have it hold its own data, but that is more
prone to leaking memory and detracts from the ease of use.
- Space between function name and parameter parenthesis, for example:
"printf ()".
Where did you see this?
- Why prefix functions in the BExperimental namespace with exp_?
I have absolutely no idea!
- Comments do not belong to the same line as code, use two lines
This seems to exclude enums, yes? Because the system universally
follows that convention.
If there is only a single extra thread per app_server instance, I
think we can certainly live with that.
Yes, only one thread per app_server.
However, I don't quite see why MessageLooper is subclassed if you use
an extra thread.
Because my original plan was to use the looper thread - and I have yet
to give up on that plan, it just doesn't want to work :-/
I've had it very close, but I just can't accept the higher CPU usage and
message count for such a simple feature.
Why not just follow the example of LinkSender, and make it an extra,
appropriately named class like DelayedLinkSender when you model it
after that one? And I'm also not sure why you modeled it after
LinkSender -- that's used for the direction of client --> server. The
other way around usually uses either LinkReceiver (synchronous) or
simply BMessages (asynchronous) for the responses. Given the target, I
would have assumed a delayed BMessage facility, not what you are doing
there.
I only modeled it after the data handling API on LinkSender (Attach<>).
I wanted the usage to be similar, not the implementation or its
functionality.
When you subclass a message looper, you make sure that this class
cannot be used client side. Maybe you can elaborate on why you put
that functionality there?
It is not meant to be used client side, it is specifically designed to
allow the delay, merging, and release of messages within the app_server
to reduce repeated messages. It is immensely useful for some messages,
completely useless for others.
For example, delaying invalidation system wide would result in almost no
benefit since the app_server already doesn't spam clients with update
requests needlessly. For the client, however, delaying how long before
the app_server does that can be immensely useful to reduce the redraw
rate (I made extensive use of this in PhOS for animations). Useful to
reduce redraws, but not to control exactly when they happen.
On the server side, its current use case will, perhaps, be one of its
most obviously beneficial. When sliding the color slider events are
fired off absurdly quickly (probably too much), and Appearance sets the
new ui color. With a system that didn't do anything other than set the
system color values, this is a known issue. I first looked into holding
it back client-side (in Appearance), but it seemed to make more sense if
the app_server could prevent spamming in the first place, rather than
relying on the clients to behave.
That still sounds like a bug in the implementation, though :-)
You don't need to have a simple FIFO queue, and I also cannot see any
reason to wake up a thread -- "you" should be that thread in the first
place in that case, no?
ServerWindow and ServerApp implement their own loop code, so I have to
inject into that code. In addition, Desktop uses the default loop, so
I'd also have to override that.
For my tests, I only derive ServerWindow from DelayedMessageLooper, and
modify ServerWindow's _MessageLooper() to use
DelayedMessageLooper::GetNextMessage().
The code is quite simple, the logic is quite simple, it's just that
somewhere, somehow, messages are not sent, or are sent very late. I'm to
the point I'm thinking it must be a bug with the timeout for
LinkReceiver, since it seems to not be used anywhere, I'm wondering how
well tested it is.
On that note, I think I saw a timeout-related bug in LinkSender's
Flush() method. As in, I don't think it would return from a timeout at
all, but I could be wrong.
Thanks, that's much better!
Oh, good!
I think I've hit the style cleanup pretty hard, examined everything in
git's diff output, and it seems like I have things covered. Obviously,
that's for you to de[r/c]ide ;-)
Here is a full diff (from master to the latest):
http://files.looncraz.net/SetViewUI-diff-2.patch
And a patchset:
http://files.looncraz.net/SetViewUI-patchset-v2.patch
This also includes a CharacterMap oversight, and corrected colors for
Deskbar's Switcher
Thank you for taking the time to look things over!
--The loon
Other related posts:
