bugGNU Octave - Bugs: bug #30519, missing "units" listener...

 
 

bug #30519: missing "units" listener for conversion of values

Submitter:  Ben Abbott <bpabbott>
Submitted:  Thu 22 Jul 2010 12:55:39 PM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  dbateman
Originator Name:  bpabbott Open/Closed:  * Closed
Release:  * dev Operating System:  * Mac OS
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 17 Sep 2010 12:33:48 AM UTC, comment #11: 

I pushed a change to ensure the text objects' position have units "data.

http://hg.savannah.gnu.org/hgweb/octave/rev/88687577519f

Ben Abbott <bpabbott>
Group Member
Thu 16 Sep 2010 11:43:48 PM UTC, comment #10: 

My understanding is that the fontunits property should only effect the value for the fontsize. The value for the "extents" property is effected by the "units" property.

I haven't checked if this is currently correct in Octave's sources, but it is clear that the Gnuplot backend is essentially ignoring the units property. When I have time I can fix that.

Perhaps some tests should be written to verify everything is working correctly. Eventually I can get around to that as well.



Ben Abbott <bpabbott>
Group Member
Thu 16 Sep 2010 09:19:54 PM UTC, comment #9: 

Ok I pushed the changeset

http://hg.savannah.gnu.org/hgweb/octave/rev/6ea65c5de87a

that addresses this as well as I'm willing to try to. Finally I used the update_* methods in the figure and axes classes rather than to try and do the conversion in the set_* and get_* function as this seemed to be what the other function have done.

There are a couple of untreated issues

- The monitorposition and pointerlocation properties of the root figure aren't respected anywhere that I can see, so I don't make these respect the root figures units property

- The text class already respects the units property. I however didn't try and make it respect the fontunits property as untangling that from the update_text_extents function didn't seem that easier.

If these points are an issue for you, please open a new feature request, as I think as the vast majority of this one being treated its no longer the relevant  place to discuss these remaining points

David Bateman <dbateman>
Group Member
Tue 07 Sep 2010 08:30:40 PM UTC, comment #8: 

Yes. The values for tightinset, position, and outerposition each have the same units.

Ben Abbott <bpabbott>
Group Member
Tue 07 Sep 2010 08:04:40 PM UTC, comment #7: 

Should the TightInset property be returned type be determined by "units"? As position and outerposition are I assume the same goes for TightInset.

D.

David Bateman <dbateman>
Group Member
Sun 05 Sep 2010 10:55:14 PM UTC, comment #6: 

The commandwindowsize is in always in characters (characters wide and lines tall).

The other three, pointerlocation, monitorposition, and screensize, are converted when the units property changes.

Ben Abbott <bpabbott>
Group Member
Sun 05 Sep 2010 09:17:51 PM UTC, comment #5: 

What does the root windows "unit" property modify? I can see the properties

commandwindowsize
pointerposition
monitorposition
screensize

that might be affected, but its not clear to me that they are affected by the "units" property. Could you check?

Thanks
David

David Bateman <dbateman>
Group Member
Sat 04 Sep 2010 10:09:49 PM UTC, comment #4: 

Sorry, I had something else on my mind. I should have given more info.

I'd assumed that when a "units",  "fontunits", or "paperunits" property is changed that Matlab converted the value of the corresponding property.

By compatibility, I meant that the property values should be handled in a manner consistent with the units property during set/get operations. That can be done when the units are changed (hence my comments regarding a listener for the units properties) or for the set/get of the property value (as you've done).

Both can produce a compatible result ... but your solution looks more efficient to me.

Ben Abbott <bpabbott>
Group Member
Sat 04 Sep 2010 09:32:58 PM UTC, comment #3: 

what do you mean "needed for compatibility"? Can you give a reference?

What my patch does is that the version of the axes position and outerposition properties that are stored are always in normalized units. However, when they are read they are converted to a value in the units given by the axis "units" property. Likewise, when they are set, the units are converted to "normalized" units from the type given by the axis "units" property before they are stored.

D.

David Bateman <dbateman>
Group Member
Sat 04 Sep 2010 07:41:52 PM UTC, comment #2: 

Listeners for all objects with a "units" property are needed for compatibility.

My c++ knowledge isn't sufficient to understand exactly what your patch does.

Does it allow for the automatic conversion of the axes position/outerposition when the axes units are changed?

If so, I assume the same approach can be applied to the root and figure objects (position and units props), as well as the axes fontsize/fontunits properties? (I just noticed I forgot to include the fontunits prop in my original comment).



Ben Abbott <bpabbott>
Group Member
Sat 04 Sep 2010 04:43:13 PM UTC, comment #1: 

Ok I fell into this on with the legend code, though I don't understand the need for the listener functions here. As far as I can see a listener would only be needed if for example changing the "units" property of an axis changed the "units" property of the figure it is associated with. What seems to be missing to me is that the {get|set}_outerposition and {get|set}_position methods of the axes class should call the convert_position function to handle the units property. The same should be done with the font* and paperposition properties for the other classes you mentioned.

The attached patch tries to address the "units" properties for the axes class and it fixes the problem I had. Does it do the right thing? Am I missing something that would mean that the listeners are needed?

If you think this is the right approach I'll fix the other classes and work this up into a changeset.

D.


(file #21388)

David Bateman <dbateman>
Group Member
Thu 22 Jul 2010 12:55:39 PM UTC, original submission:  

There is some existing code for units conversion in graphics.cc (convert_position). However, there is no listener to make use of it when the "units" properties of various objects are changed.

The objects and units which need to be handled are ...

root
      radio_property units U , "inches|centimeters|normalized|points|{pixels}"
figure
      radio_property paperunits , "{inches}|centimeters|normalized|points"
      radio_property units , "inches|centimeters|normalized|points|{pixels}|characters"
axes
      radio_property units , "{normalized}|inches|centimeters|points|pixels|characters"
text
      radio_property units u , "{data}|pixels|normalized|inches|centimeters|points"





Ben Abbott <bpabbott>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #21388:  patch.units added by dbateman (3KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dbateman (Updated the item)
  • -email is unavailable- added by bpabbott (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-09-16 dbateman StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2010-09-06 dbateman StatusNone In Progress
        Assigned toNone dbateman
    2010-09-04 dbateman Attached File- Added patch.units, #21388

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code