 |
|
|
|
|
|
| |
| |
|
|
|
|
| |
| |
|
|
In povray-3.50c's unix.cpp, function HandleXEvents(), there is
some code which reads as follows (line 3382):
-----------------------------------------------------------------------
else if (theEvent->xkey.state | ControlMask && (theKeySym == XK_L ||
theKeySym == XK_l || theKeySym == XK_R || theKeySym == XK_r))
{
refresh_y_min = 0;
refresh_y_max = Frame.Screen_Height;
}
-----------------------------------------------------------------------
This is a bug. It must be:
-----------------------------------------------------------------------
else if ((theEvent->xkey.state & ControlMask) && (theKeySym == XK_L ||
theKeySym == XK_l || theKeySym == XK_R || theKeySym == XK_r))
{
refresh_y_min = 0;
refresh_y_max = Frame.Screen_Height;
}
-----------------------------------------------------------------------
Notice the '|' before ControlMask; put brackets around it for clarity.
Please fix that in 3.51.
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
> This is a bug. It must be:
Can you clarify the nature of the problem you're fixing ?
- NC
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Nicolas Calimet wrote:
>> This is a bug. It must be:
>
> Can you clarify the nature of the problem you're fixing ?
>
Sorry, but the presence of a flag is tested by using
state & flag
and not
state | flag
because the latter is always true.
Hence, writing
if(theEvent->xkey.state | ControlMask ....
is a bug and it must be
if(theEvent->xkey.state & ControlMask ....
just from reading the code.
So, in order to comply with the description saying that Control-L
does a complete screen re-draw (as usual in Unix), one has to
apply the change I mentioned.
I was not aware that any clarification was needed for that...
...or what is the problem?
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Wolfgang Wieser wrote:
> Nicolas Calimet wrote:
>
>>>This is a bug. It must be:
>>
>>Can you clarify the nature of the problem you're fixing ?
>>
>
> Sorry, but the presence of a flag is tested by using
> state & flag
> and not
> state | flag
> because the latter is always true.
Does the phrase "if it ain't broken, don't fix it" mean anything to you?
I think Nicolas was asking for and explanation why to apply this change.
Something like 'in case of the xxx situation POV-Ray does yyy which is
not correct'.
Christoph
--
POV-Ray tutorials, include files, Sim-POV,
HCR-Edit and more: http://www.tu-bs.de/~y0013390/
Last updated 25 Oct. 2003 _____./\/^>_*_<^\/\.______
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
> Sorry, but the presence of a flag is tested by using
> state & flag
> and not
> state | flag
> because the latter is always true.
No. The latter is false when both state and flag are false.
It's true in any other case.
Maybe it's always true in the context of the povray code
you report, but honestly I have no idea what can be the values
of theEvent->xkey.state and ControlMask.
> I was not aware that any clarification was needed for that...
> ...or what is the problem?
When fixing a problem, you'd better give a description
of the problem _first_ :-)
I suppose you found that CTRL+L was not refreshing the
screen as expected. If your fix does solve the problem, then
it will be included in the next povray version after testing.
- NC
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Christoph Hormann wrote:
> Wolfgang Wieser wrote:
>> Nicolas Calimet wrote:
>>
>>>>This is a bug. It must be:
>>>
>>>Can you clarify the nature of the problem you're fixing ?
>>>
>>
>> Sorry, but the presence of a flag is tested by using
>> state & flag
>> and not
>> state | flag
>> because the latter is always true.
>
> Does the phrase "if it ain't broken, don't fix it" mean anything to you?
>
This phrase does not reflect my philisophy.
And -- if you read code like that:
if(state | StateBit) { do sth }
What would you think about the author? -- Oh dear...
> I think Nicolas was asking for and explanation why to apply this change.
>
To ask for "the nature of the problem" sounded to me like complete
lack of understanding... like a "tell me what's going on here?!".
I was really puzzled -- I thought for a UNIX guy like Nicolas
the point should be fairly clear.
> Something like 'in case of the xxx situation POV-Ray does yyy which is
> not correct'.
>
I provided the answer, IMO -- quoting myself:
So, in order to comply with the description saying that Control-L
does a complete screen re-draw (as usual in Unix), one has to
apply the change I mentioned.
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Nicolas Calimet wrote:
>> Sorry, but the presence of a flag is tested by using
>> state & flag
>> and not
>> state | flag
>> because the latter is always true.
>
> No. The latter is false when both state and flag are false.
> It's true in any other case.
>
Oh dear... I was doing raw Xlib programming, so thinks like
these are familiar to me:
bash# fgrep -r ControlMask /usr/include/X11/*.h
/usr/include/X11/X.h:#define ControlMask (1<<2)
> Maybe it's always true in the context of the povray code
> you report, but honestly I have no idea what can be the values
> of theEvent->xkey.state and ControlMask.
>
It's simple: When the control key is pressed, the ControlMask
bit is set on the state value. Otherwise not. To test for
pressed control key, one checks for if(state & ControlMask)
>> I was not aware that any clarification was needed for that...
>> ...or what is the problem?
>
> When fixing a problem, you'd better give a description
> of the problem _first_ :-)
>
Yeah -- and I thought everyone would see the point after looking
just 2 seconds at the code. It was just soooo evident to me.
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Wolfgang Wieser wrote:
>
> Yeah -- and I thought everyone would see the point after looking
> just 2 seconds at the code. It was just soooo evident to me.
>
Please understand that also for a user interface bug the basic rules of
bug reporting apply, i.e. clear description of the symptoms of the
problem is required. If you had said you tested that this change fixes
a lack of response to ctrl-l in POV-Ray everything would have been clear.
Christoph
--
POV-Ray tutorials, include files, Sim-POV,
HCR-Edit and more: http://www.tu-bs.de/~y0013390/
Last updated 25 Oct. 2003 _____./\/^>_*_<^\/\.______
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
> bash# fgrep -r ControlMask /usr/include/X11/*.h
> /usr/include/X11/X.h:#define ControlMask (1<<2)
Okay, in this case I agree it's always true :-)
> Yeah -- and I thought everyone would see the point after looking
> just 2 seconds at the code. It was just soooo evident to me.
Sure, and I actually got the point when reading the code
(but was still too lazy to grep for ControlMask). My first post
was simply intended to pass the message that giving a clear & simple
description of the problem may help saving time. However, this
message was not clear either... ;-)
Anyway, thanx for fiding this. The Xwindows code would
actually need more extensive review, so if you find any other
problem which is not related to Xwindows-specific command-line
options (this part of the code is quite buggy; currently fixed),
you're welcome to report !
- NC
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
In article <3fc3c9ad@news.povray.org> , Wolfgang Wieser <wwi### [at] gmx de>
wrote:
>>> Sorry, but the presence of a flag is tested by using
>>> state & flag
>>> and not
>>> state | flag
>>> because the latter is always true.
>>
>> Does the phrase "if it ain't broken, don't fix it" mean anything to you?
>>
> This phrase does not reflect my philisophy.
>
> And -- if you read code like that:
> if(state | StateBit) { do sth }
> What would you think about the author? -- Oh dear...
Compared to code used in some commercial applications, the code in POV-Ray
is almost perfect. ;-)
Sadly, for any project that is big or complex enough you cannot say much
about the authors if you just look at the code when it is 15 years old and
has been treated by dozens of people. Even if it would be a new project, at
best you can say how good the project management was, but still very little
about the authors.
Saying something about one particular author requires to make sure only one
author ever touched the code. If you are lucky you find such condition in
an entry-level programming course, not anywhere else :-(
Thorsten
____________________________________________________
Thorsten Froehlich, Duisburg, Germany
e-mail: tho### [at] trfde
Visit POV-Ray on the web: http://mac.povray.org
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
In article <3FC### [at] freefr>,
Nicolas Calimet <pov### [at] freefr> wrote:
> > Sorry, but the presence of a flag is tested by using
> > state & flag
> > and not
> > state | flag
> > because the latter is always true.
>
> No. The latter is false when both state and flag are false.
> It's true in any other case.
Of what use would this be? And even then, it wouldn't work
properly...such a flag could never be set, but state | flag would be
true if any other flag is set.
--
Christopher James Huff <cja### [at] earthlinknet>
http://home.earthlink.net/~cjameshuff/
POV-Ray TAG: chr### [at] tagpovrayorg
http://tag.povray.org/
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
>>> state | flag
>>>because the latter is always true.
>>
>> No. The latter is false when both state and flag are false.
>>It's true in any other case.
>
> Of what use would this be?
None. I was simply noticing that the statement:
"state | flag is always true" is not correct in the general
sense (i.e. as written here).
- NC
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Thorsten Froehlich wrote:
> In article <3fc3c9ad@news.povray.org> , Wolfgang Wieser wrote:
>> And -- if you read code like that:
>> if(state | StateBit) { do sth }
>> What would you think about the author? -- Oh dear...
>
> Compared to code used in some commercial applications, the code in POV-Ray
> is almost perfect. ;-)
>
> Sadly, for any project that is big or complex enough you cannot say much
> about the authors if you just look at the code when it is 15 years old and
> has been treated by dozens of people. Even if it would be a new project,
> at best you can say how good the project management was, but still very
> little about the authors.
>
> Saying something about one particular author requires to make sure only
> one
> author ever touched the code. If you are lucky you find such condition in
> an entry-level programming course, not anywhere else :-(
>
You misunderstood me...
I just wanted to make a point stating that the above code should
be fixed even "if it ain't broken"...
(For the same reson why people fix typos in a book even if you can read
the book _with_ the typos.)
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Christoph Hormann wrote:
> Please understand that also for a user interface bug the basic rules of
> bug reporting apply, i.e. clear description of the symptoms of the
> problem is required. If you had said you tested that this change fixes
> a lack of response to ctrl-l in POV-Ray everything would have been clear.
>
I completely understand you.
Just to give you the background:
The bug was _not_ discovered by observing misbehaviour. I just had
a casual look at unix.cpp while writing my PRT patch and that
particular line just jumped on me. It was so evident to me that this
was a bug because from doing XLib programming for years,
XEvent.state and ControlMask were familiar to me and so I thought
any deeper explanation would just bore the maintainer...
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
Nicolas Calimet wrote:
> Anyway, thanx for fiding this. The Xwindows code would
> actually need more extensive review, so if you find any other
> problem which is not related to Xwindows-specific command-line
> options (this part of the code is quite buggy; currently fixed),
> you're welcome to report !
>
Okay, that's no problem -- If I find a bug and have a fix, I always
report it. In case I don't have a fix, I report only severe bugs.
It would, however, be helpful if you gave me the current development
version of POVRay.
Wolfgang
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
| |
|
|
> It would, however, be helpful if you gave me the current development
> version of POVRay.
Apart the obvious bugfixes in XWIN_init_povray() that I mentioned
before, the current Xwindows-specific code is still the same as in 3.50c,
so reviewing the latter would be for sure of a great help.
- NC
Post a reply to this message
|
|
| |
| |
|
|
|
|
| |
