taurus-org / taurus

Moved to https://gitlab.com/taurus-org/taurus
http://taurus-scada.org
43 stars 46 forks source link

Maintain old widget for 0D and PseudoCounters in TaurusValue #1101

Closed reszelaz closed 4 years ago

reszelaz commented 4 years ago

sardana-org/sardana#1203 introduces new widget for experimental channels which allows acquisition. Since 0D and PseudoCounters do not allow experimental channel acquisition still use the old widget for them.

reszelaz commented 4 years ago

Hi Carlos, Could you please check what is wrong with the testsuite, especially for py3-qt5? It seems like there are some core tests failing and I think that this PR changes should not affect these tests. Thanks a lot!

cpascual commented 4 years ago
reszelaz commented 4 years ago

Great! Thanks for checking that! Then do you think we could merge it (merging at the same time https://github.com/sardana-org/sardana/pull/1203)? Is there any other place where the TV map is declared or the custom settings module is the only one?

cpascual commented 4 years ago

IMHO, a better solution is to add some transitional logic in the new PoolChannelTV to support the 0D and PseudoCounters as part of https://github.com/sardana-org/sardana/pull/1203

Let me elaborate: I am not saying that the complete support of 0D and PseudoCounters should be added now as part of https://github.com/sardana-org/sardana/pull/1203 . What I mean is that some check is temporarily implemented in the setModel of the new PoolChannelTVthat disables the new features if the model corresponds to a 0D or PseudoCounter.

cpascual commented 4 years ago

Is there any other place where the TV map is declared or the custom settings module is the only one?

I've checked and I'd say that tauruscustomsettings is the only place (apart from some irrelevant "demo" code).

Still, I think it is worth considering making this setting an entry-point to get rid of this sardana- and tango- centric bit in taurus. This is already identified in APPENDIX IV of TEP13.

If we implemented this entry-point, then it would be straight away for sardana-org/sardana#1203 to register whatever map it needs for its own widgets (without worrying about side-efects in taurus).

So, I see 3 alternatives for achieving the objectives of this PR without breaking compatibility:

  1. temporarilly adding a tango class check in the new PoolChannelTV.setModel() to handle the same classes as the old one
  2. monkey-patching from sardana to update tauruscustomsettings.T_FORM_CUSTOM_WIDGET_MAP
  3. implementing an entry-point for updating the custom widget map of a taurusform (and leaving the current tauruscustomsettings map as a deprecated fallback)
reszelaz commented 4 years ago

Many thanks for this in-depth analysis. I have not thought about the scenario of upgrading taurus only and in this case you are right - users will lost the PoolChannelTV (PCTV) in favor of TaurusValue for 0D and pseudo counters.

However I don't see it terrible for the following two reasons:

  1. The main difference between these two widgets is that the old PCTV allows you to see at the first sight the Value attribute with a label widget. It is not very usefull cause to make it change you need to run a macro e.g. either a ct or a scan. Then if you run the macro you already see the updated value on the macro output. Well, of course there are other less relevant features of this widget e.g. the Value label shows in the background the attribute's quality.
  2. The tauruscustomsettings.T_FORM_CUSTOM_WIDGET_MAP is a custom settings, so one always has the possibility to recover the previous behavior by changing this setting.

From the 3 options that you identify I'm in favor of putting all the efforts on 3. I will start disucssion on this option in a separate issue, however I would not block integration of these PRs. What do you think?

cpascual commented 4 years ago

The main difference between these two widgets is that the old PCTV allows you to see at the first sight the Value attribute with a label widget

Exactly, but I think that that is annoying for someone who already has a working GUI and it suddenly it changes its appearance after updating taurus (again, consider py2 users, which are still supported in taurus)

The tauruscustomsettings.T_FORM_CUSTOM_WIDGET_MAP is a custom settings, so one always has the possibility to recover the previous behavior by changing this setting.

But you realize that this works also as an argument against doing this change, right? ;)

I would not block integration of these PRs

Actually, I think it makes sense to block it. I think it is such a small change that it is not worth breaking taurus even if slightly and/or temporaly.

I'll do a PR ASAP (today) with the entry-point solution. And, in order to speed-up things, I can even advance the API:

in sardana you will be required to register the following:

entry_points = {
    "taurus.qt.qtgui.taurusform.custom_widget_map": ["<name> = <mod>:<dict>"],
    (...)
}

where <name> can be any string (I suggest sardana), and <mod>:<dict> points to a dictionary defined in some sardana submodule with the same syntax as T_FORM_CUSTOM_WIDGET_MAP and containing only the entries handled by sardana widgets (for example,sardana.sardanacustomsettings:T_FORM_CUSTOM_WIDGET_MAP)

reszelaz commented 4 years ago

I think there is no need to maintain this PR opened since the entry_point solution fixes this. Thanks for the review!