[haiku-development] Re: My SetViewUI ... patch
- From: looncraz <looncraz@xxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Tue, 6 Oct 2015 12:26:13 -0500
On 10/6/2015 10:11, Axel Dörfler wrote:
It's like a big wart when something sticks out from a coding style I'm
used to -- I don't even have to look close ;-)
I can understand that, my style is just quite drastically different,
favoring spacing, standard abbreviations, reduced line wrapping (no 80
char limit), and absolutely no need to name obvious parameters in
headers... for starters ;-)
I didn't mean that you should remove all comments. Just superfluous
ones, and those, that belong into the doxygen files. Documentation is
important.
I'll document the final version fully, I just removed the comments that
weren't really helping comprehension of the code. The rest of the file
is pretty barren, comment-wise, so I followed suit.
> rgb_color mix_color(rgb_color color1, rgb_color color2, uint8 amount)
(divide into two lines)
I really have no idea what you mean here.
C++ comments.
I'm bad at that...
Superfluous blank line (in a number of cases).
I'm wonderfully bad at that as well :-/
I also spotted the use of "new" without nothrow in the app_server --
you even checked against NULL afterwards, but that doesn't have any
effect at this point.
EEK! I *KNOW* better, LOL!
Also, you should really try to create cleaner patches: when you clean
up the coding style, you should *only* clean up the coding style.
Don't change code, don't insert a BAutolock somewhere when it wasn't
there before. When you can't behave while writing the code, "git add
-i" is your friend.
Thought I did, probably just forgot to commit some changes before doing
the style cleanup. My bad, will be more careful!
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.
I must admit that I don't understand the distinction between these
two, or rather, the reason for it. It just seems like extraneous code.
The DelayedMessage is a facility for building the data that will be sent
as a message. It is a throw-away object that doesn't own or copy any
data itself. It helps to ensure that the original attached data is
actually never copied if the message is merged, and new memory is only
allocated if merging fails (this could be easily done in other ways, of
course). It also helps to separate the responsibilities of the looper
from the messages (the looper being responsible for scheduling and,
originally, dispatching, the messages being responsible to ensure their
data is attached to a sender and sent to the correct place). But, above
all, it makes it much easier and cleaner to use.
DelayedMessage delayed(...);
delayed.Attach<>();
...
ScheduleMessage(delayed);
break;
vs
DelayedMessage* delayed = new(std::nothrow) DelayedMessage(...);
if (delayed == NULL)
break;
delayed->Attach<>();
...
if (ScheduleMessage(delayed) != B_OK)
delete delayed;
break;
When looking at the code it is helpful to keep in mind that I wrote this
with the idea that future fast-repeating messages could be merged easily
and that it would just be a little facility integrated with
MessageLooper. I'm still clinging to that idea :p
I haven't given up getting rid of the thread, if you don't want to
accept it with it still existing, I can totally accept that, I'll keep
digging to see if I found a bug with LinkReceiver's handling of the
timeout (first glance at the code didn't show anything immediately
obvious). One way or another, I will eventually get rid of that thread!
Another idea I had was to just find a way to reduce the number of
updates coming from BColorControl. But my mind might enjoy complexity a
bit too much, I admit. That, and I have no idea how to accomplish that
goal. I seem to remember a discussion on the list a few years ago about
that very topic and no solution ever being found.
- Space between function name and parameter parenthesis, for example:
"printf ()".
Where did you see this?
A number of times, actually.
> + bool operator < (const DMScheduledMessage&
other) const;
Would be one occasion, but there was more. You definitely fixed some
of those in your cleanup patches, though. I think I spotted one in
some header.
Ah, yes, not with the calls, though, like your original example, but
with the declarations - yeah, I'm REALLY good at doing that.
- Why prefix functions in the BExperimental namespace with exp_?
I have absolutely no idea!
Now that you remember: why not just remove them, and put their meat
into rgb_color directly?
They exist in ColorTools.cpp which has a nice OpenTracker license
attached. Wasn't sure if I should just remove that file. Actually
meant to ask, got distracted, and now we're here.
But, by "into rgb_color directly," do you mean something such as
rgb_color::MixWithColor(rgb_color other, uint8 amount)?
When you need more CPU then there is something wrong :-)
Exactly. I'm going to roll this out into userland again and see if I
can replicate the issue there, if not, it will really narrow down the
possible culprit of my issues... I've done far too much threading in my
life for something this simple to be so troublesome.
That's actually a good reason to have it in the client, too. Sending a
message to the app_server that will then remember to send itself
another message, in order to send a message back to the app seems like
quite more work and detours as needed.
True, on the client I'd simply do it as follows:
The first time a color is changed in Appearance the color_which value
would be set on an atomically test-and-set color_which member variable
only when it equals B_NO_COLOR, or is the same, and a thread would be
launched that would only allow setting the color every 8ms or so (120hz
rate). The thread would stay alive and keep polling the variable to see
if it was anything other than B_NO_COLOR.
If you'd prefer this way, then I'll jump right on it and remove all the
DelayedMessage code from the app_server entirely, though we will lose
DelayedInvalidate() at the same time... but there are ways around that,
naturally, but I think we'd lose a very valuable facility....
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.
Why not simply use a BMessageRunner for animations? Should work pretty
much in the same way.
DelayedInvalidate(8000); is a LOT easier than all the effort that goes
into using a BMessageRunner. In fact, I've made it a point in life to
never use it again if I can avoid it.
For example, you can use DelayedInvalidate() right in Draw() very safely
while still having some measure of redraw rate control.
Take this psuedo-code example for pulsing the navigation color or
something (I actually have a really nice idea with this, but I'm
focusing on this and font awareness first and foremost).
void
BTextControl::Draw(BRect upateRect)
{
... normal drawing code...
if (HasFocus()) {
... draw next color outline/effect
DelayedInvalidate(15000, effectRect);
}
}
I bet that's how Be did it and is why Dan0 had the method.
Be's implementation didn't merge repeats, though, so if you ran a tight
loop with DelayedInvalidate() calls, it you'd get each one of those
invalidations later.
My current implementation would mean that interim redraws won't result
in repeated firing of draw calls if they were identical, and the
existing code would possibly merge the rects as well, obviously.
The utility of this can't really be understated, once you use it a few
times, you really won't want to go back.
That said, this could also be accomplished client-side, but I really
don't have a good idea as to the best way of doing so.
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().
Not sure what you are getting at, since all subclass MessageLooper.
Apart from that, I think for a small limited feature like this the
code is quite invasive.
They subclass it, but then don't use its existing looper code at all,
they fully override it. Shouldn't really be a problem, though, so long
as DelayedMessageLooper gets first dibs at the message troph, which
requires only changing receiver.GetNextMessage(code) to
DelayedMessageLooper::GetNextMessage(code). If I can get rid of
DelayedMessageThread I'd be much happier.
The computational complexity of the DelayedMessage facilities, however,
is rather deceptive. In the end, a tiny throw-away object is created,
data is only copied once ONLY IF the data can't be merged (which it
usually can), and the amount of communications it saves can be drastic,
so it's a code-heavy, but deceptively lightweight, utility. Personally,
I'd leave it in for DelayedInvalidate alone. Being able to also crush
repeated set_ui_color calls and possibly other future heavy-hitting
calls just adds to the utility.
I'm not attached to the implementation, just what it accomplishes. Any
better ideas are very welcome.
The great thing is that it doesn't effect compatibility, so we can move
forward and replace it with whatever better idea comes along later.
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 ;-)
Looks like our checker tools need to become much better.
Would be helpful if they could go over a patch and recognize only the
lines which need to be examined. Running them on the patch itself
generated a list of faults four times larger than the patch file... I
didn't expect anything different, I was just being hopeful ;-)
Thank you for taking the time to look things over!
That's the spirit! Thanks for not letting this stop you :-)
Stop me? Ha!
I just have to make writing Haiku style-appropriate code second nature,
that takes practice :p
--The loon
Other related posts:
