Opened 4 years ago
Closed 4 years ago
#6127 closed defect (fixed)
Improve drop-down menu for replaceable class
Reported by: | Francesco Casella | Owned by: | Adeel Asghar |
---|---|---|---|
Priority: | blocker | Milestone: | 1.16.0 |
Component: | OMEdit | Version: | |
Keywords: | Cc: | Adrian Pop, Andrea Bartolini, Adeel Asghar |
Description
Please consider the attached test package. Enable replaceable support, load it, open the model S1
and double-click on m
m.RM
has a default typeTestReplaceableComments.RM1
. This is not visible, so it seems that something's missing here. We should display the default type in grey as we do for the default values of parameters, see e.g. the value ofp
in the same window.- the entries of the drop-down menu contain the Modelica code "redeclare model RM = XXX". Maybe it would be better to skip the "redeclare model RM = " part, and rather show the class names and their comments, which normally contain useful information to identify what the class is good for
- the last item of the redeclarable classes is "TestReplaceableComments.M.RM". As far as I understand, according to Section 5.3.2 of the specification, this shouldn't be there, because composite name lookup inside non-package classes should be restricted to encapsulated elements.
Last, but not least, if you now open S2
and double click on m
, you should actually see that RM
has already been redeclared to RM2
, the same way p
was changed to 5.
Attachments (1)
Change History (14)
comment:1 by , 4 years ago
Cc: | added |
---|
by , 4 years ago
Attachment: | TestReplaceableComments.mo added |
---|
comment:2 by , 4 years ago
Milestone: | Future → 1.16.0 |
---|---|
Priority: | high → blocker |
comment:3 by , 4 years ago
comment:4 by , 4 years ago
Looks good, when I open the parameter window of m
in S2
I see the redeclared class.
comment:5 by , 4 years ago
Regarding S2
, it seems to work on Ubuntu 18.04 (when I open the parameter window of m
in S2
I see the redeclared class).
Regarding S1
, all the issues listed above are still present.
comment:7 by , 4 years ago
@adrpo, this is much improved, but we still have a problem: you can't distinguish the case where there is no redeclare from the other ones. When you double-click on m
in S1
, you see Replaceable model 1 as if there was actually a redeclare statement, which is in fact not there; what you are seeing is the original element which has not been redeclared. That should be displayed in grey, as with default parameter values if you don't change them.
Along the same line, when you click on the drop-down menu, there should also be an empty entry, and if you click on it, no redeclare should be applied.
Minor comment: I would probably remove the quotation marks and just put the text of the comment alongside the class name. It is easy to distinguish them because they are separated by the first space in each menu entry.
follow-up: 9 comment:8 by , 4 years ago
Cc: | added |
---|
As I wrote in the PR: "not sure how to make greyed display value in a comobox but I greyed it out in the list". I guess @adeas31 can probably help us with this.
comment:9 by , 4 years ago
Replying to adrpo:
As I wrote in the PR: "not sure how to make greyed display value in a comobox but I greyed it out in the list". I guess @adeas31 can probably help us with this.
OK. In fact, I saw the blank line in the drop-down list, but that's not appropriate; a blank line should be provided instead if one wants to remove the redeclare and get back to the default behaviour.
EDIT: of course I meant the greyed-out line, sorry
comment:10 by , 4 years ago
Is not the blank line, the item is grey in the list. I talked with @adeas31 who gave me some pointers. I will try that and see how it goes.
comment:11 by , 4 years ago
This PR fixes some of the issues:
but greying out the default doesn't work yet, I asked @adeas31 for help with this one.
comment:13 by , 4 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I tested this in the 1.17.0 nightly and it looks very good. I understand 1.16.0-dev.beta4 is basically the same, so I'm closing this ticket. I'll test the beta tomorrow anyway.
Partially fixed via:
https://github.com/OpenModelica/OpenModelica/pull/6777
default choice is now populated.