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

 
 

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

Submitted by:  Ben Abbott <bpabbott>
Submitted on:  Thu 22 Jul 2010 12:55:39 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: David Bateman <dbateman>
Originator Name: bpabbottOpen/Closed: Closed
Release: devOperating System: Mac OS

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>
Project Member

 

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

Attach File(s):
   
   
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
  • -unavailable- added by dbateman (Updated the item)
  • -unavailable- added by bpabbott (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 16 Sep 2010 09:19:54 PM UTCdbatemanStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Mon 06 Sep 2010 09:29:39 AM UTCdbatemanStatusNone=>In Progress
      Assigned toNone=>dbateman
    Sat 04 Sep 2010 04:43:13 PM UTCdbatemanAttached File-=>Added patch.units, #21388

    Back to the top


    Powered by Savane 3.1-cleanup1