Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

Spinbox was showing units even if the model use fragments to display only the magnitude #669

Merged
merged 6 commits into from
Jan 23, 2018

Conversation

srgblnch
Copy link

I've seen that a TaurusValueSpinBox was still showing the units in widgets where the model say, in the fragment, that only the magnitude was to be shown.

This pull request contains 2 commits. The first one was changes in the main function at the end of the file, for testing purposes (as well as a pep8 review). Then, like the main in the tauruslabel one can pass some attributes as arguments.

The commit with the solution itself is the second. There is a single change in the TaurusValueLineEdit.setValue(self, v). It was using self.displayValue(v) when the method that manages with the fragments is getDisplayValue.

/Sergi.

@cpascual
Copy link
Member

The proposed implementation has one problem: now by default the shown value is rvalue (while in this widget it shoud be wvalue).

This can be checked easily with an attribute such as sys/tg_test/1/short_scalar whose wvalue and rvalue are not the same:

from taurus.qt.qtgui.application import TaurusApplication
from taurus.qt.qtgui.input import TaurusValueLineEdit

if __name__ == '__main__':
    import sys
    app = TaurusApplication()

    w = TaurusValueLineEdit()

    model = 'sys/tg_test/1/short_scalar'
    w.setModel(model)

    w.show()
    sys.exit(app.exec_())

@srgblnch
Copy link
Author

Then I will need a hint. TaurusBaseComponent.getDisplayValue(...) seems to only manage the fragment thing, to then call the TaurusBaseComponent.displayValue(...) that the TaurusValueLineEdit.setValue(...) was calling before.

Using the modification I've made in the main of the tauruslineedit.py to allow just this testing, I call:
$ python lib/taurus/qt/qtgui/input/tauruslineedit.py sys/tg_test/1/short_scalar sys/tg_test/1/short_scalar#rvalue.magnitude sys/tg_test/1/short_scalar#wvalue.magnitude

And yes, when the patched code is called without a fragment the value shown is the rvalue. This may play with the fragmentName argument? Sorry but the help in the methods is not helping me to understand them.

@srgblnch
Copy link
Author

Perhaps the TaurusBaseWritableWidget.setModel(...) (that is not implemented and is using the one in the superclass TaurusBaseComponent) should modify the defaultFragmentName to change on the writable things from 'rvalue' to 'wvalue'?

…and tag its superclass from where all the writable widgets should inherit to be reviewed by the developers core.
@srgblnch
Copy link
Author

Would this third commit be good enough to prevent the behaviour of showing the rvalue when no fragment is shown?

By the way, the bugfix doesn't interfere the superclass, as we have spoken, in order avoid an API impact.

@@ -201,6 +201,29 @@ def _stepBy(self, v):
value = self.getValue()
self.setValue(value + Quantity(v, value.units))

def getDisplayValue(self, cache=True, fragmentName=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not critical , but... most taurus code avoids using super because of some issues with diamond inheritance schemes, so I would prefer the following implementation:

def getDisplayValue(self, cache=True, fragmentName=None):
    if fragmentName is None and self.modelFragmentName is None:
        fragmentName = 'wvalue'
    return TaurusBaseWidget.getDisplayValue(self, cache=cache, fragmentName=fragmentName)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this TaurusValueLineEdit.getDisplayValue() may call the newer TaurusBaseWritableWidget.getDisplayValue() instead of the supersuperclass. Even it's an empty call right now, if this is propagated up one would realise better the scale.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% right. It should be TaurusBaseWritableWidget.getDisplayValue

My mistake

# instead of a bugfix.
# The bugfix impact has been bounded only within the
# TaurusValueLineEdit.
return super(TaurusBaseWritableWidget,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid using super (see my other comment)

@cpascual cpascual added this to the Jan18 milestone Jan 19, 2018
@cpascual cpascual added the bug label Jan 19, 2018
@cpascual
Copy link
Member

Merging, with two minor changes.

Thanks @srgblnch !

@cpascual cpascual merged commit eea2f58 into taurus-org:develop Jan 23, 2018
@srgblnch srgblnch deleted the spinboxUnits branch January 23, 2018 08:45
cpascual pushed a commit that referenced this pull request Feb 28, 2018
The step up/down feature of TaurusValueLineEdit (e.g. using
the up/down keys or the arrows of a TaurusValueSpinBox) was broken when
merging #669 which implemented fragments support in
TaurusValueLineEdit for allowing the display of the wvalue without units
Now we see that supporting fragments generically in TaurusValueLineEdit
is problematic (e.g. it makes little sense to use "rvalue" as a
fragment since "wvalue" will be modified when eventually applying).
Therefore for now limit the fragment support in line edits
to the "wvalue.magnitude" case.

Also fix a hook error introduced in some previous merge.
cpascual pushed a commit that referenced this pull request Feb 28, 2018
The step up/down feature of TaurusValueLineEdit (e.g. using
the up/down keys or the arrows of a TaurusValueSpinBox) was broken when
merging #669 which implemented fragments support in
TaurusValueLineEdit for allowing the display of the wvalue without units
Now we see that supporting fragments generically in TaurusValueLineEdit
is problematic (e.g. it makes little sense to use "rvalue" as a
fragment since "wvalue" will be modified when eventually applying).
Therefore for now limit the fragment support in line edits
to the "wvalue.magnitude" case.

Also fix a hook error introduced in some previous merge.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants