Skip to content

Fix for #541 - use Model field to identify the Equation component#548

Merged
guitorri merged 1 commit intoQucs:release-0.0.19from
in3otd:issue_541_fix
Oct 30, 2016
Merged

Fix for #541 - use Model field to identify the Equation component#548
guitorri merged 1 commit intoQucs:release-0.0.19from
in3otd:issue_541_fix

Conversation

@in3otd
Copy link
Copy Markdown
Contributor

@in3otd in3otd commented Jul 24, 2016

The Up/Down buttons in the Component Properties dialog for reordering the equations in the Equation component are now enabled looking at the component Model field and not at its textual description, so it works independently of the selected language for the GUI.

As said elsewhere, it certainly not future proof and there are currently a lot of other places where the component Model field is used to identify a particular component.

in the Component Properties dialog. Previous usage of the Description
did not work with translations and anyway the Description is not a
unique identifier.
@felix-salfelder
Copy link
Copy Markdown
Member

thanks for the seperate PR.

we had a lengthy discussion already. let me summarize

  • the reason for the problem is the string comparison madness
  • Module has been staged for removal, and that's work in progress
  • i strongly suggest to merge that other PR first
  • i suggested a solution in some line note of an outdated commit of yours.

mainly, putting Module here instead of Description is rearranging the deck chairs on the titanic. it does not work. if this approached worked, we would not talk about a fix, right?

@in3otd
Copy link
Copy Markdown
Contributor Author

in3otd commented Jul 24, 2016

yes, we had a lengthy discussion already, but still seems to me not much relevant to this PR.

My summary is:

Even shorter summary:

  • this fix works now and does not make future refactorings any harder.

@felix-salfelder
Copy link
Copy Markdown
Member

removing Description and Model from the components in release-0.0.19 seems a big change to me and certainly I do not know how to do that properly anyway.

we don't need to remove Model entitirely just to get it right in one place. and that was not what i was proposing.

this fix works now and does not make future refactorings any harder.

yes it does. how much refactoring have you done?

@in3otd
Copy link
Copy Markdown
Contributor Author

in3otd commented Jul 24, 2016

yes it does. how much refactoring have you done?

nothing related to Description and Model for sure. But what has the question to do with this PR making the refactoring harder?
The line I changed was intended to identify an Equation component, but wrongly used the Description field to do so. The refactoring will have to handle this case anyway in a different way, even without this PR. Unless if the refactoring is blindly done to reflect the current code and will not address fixing the bug.

You seems to have a better way to solve #541, great, just make a PR.

@felix-salfelder
Copy link
Copy Markdown
Member

But what has the question to do with this PR making the refactoring harder?

fixing Model abuse means removing all occurences of Model. pl^H^Hfeel free to join in.

You seems to have a better way to solve #541, great, just make a PR.

just backported the fix to #383. this PR also cleans up related problems.

@guitorri guitorri added this to the 0.0.19 milestone Oct 28, 2016
@guitorri
Copy link
Copy Markdown
Member

One more line to change during refactor... Merging as it fixes the issue in question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants