Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra error: "The ODF does not match the combination file" #809

Closed
oleg68 opened this issue Oct 29, 2021 · 13 comments · Fixed by #811
Closed

Extra error: "The ODF does not match the combination file" #809

oleg68 opened this issue Oct 29, 2021 · 13 comments · Fixed by #811
Assignees
Labels
bug Something isn't working

Comments

@oleg68
Copy link
Contributor

oleg68 commented Oct 29, 2021

When I try to import a combination saved from a previous version of ODF, the combination is loaded successfully, but the error message "The ODF does not match the combination file" appears in the log window.

  1. I import a combination into the organ, not the organ into the combination. So this message is not precise. The combination file does not match the current ODF sounds more precisely.
  2. Because the combination is loaded anyway, the message should be a warning, not an error.
@larspalo
Copy link
Contributor

I think we absolutely should change that message! Also related to #807 is the fact that if the ChurchName differs, you first get a log message warning This combination file is only compatible with:, that I suggest we alter to This settings file was originally made for: and then you get the above message and no matter the yes/no selection the settings won't be imported (because they are only loaded if the ChurchName is the same). As for #807, I think that we should allow importing settings files with different ChurchNames, after user confirmation of course.

I can make a PR for both the issues.

@larspalo larspalo self-assigned this Oct 29, 2021
@oleg68
Copy link
Contributor Author

oleg68 commented Oct 29, 2021

@larspalo In my case the church name remains the same, but there are another changes in ODF, for example, Pipe999Gain or Pipe999PitchTuning.

The messages you suggested won't be clear in this case.

@larspalo
Copy link
Contributor

In my case the church name remains the same, but there are another changes in ODF, for example, Pipe999Gain or Pipe999PitchTuning.

For such changes the current .cmb import won't work, unless it's a completely "new" stop/rank you're adding. If you as in your case modify an already existing stop, you would need to have the ability to import selectively from .cmb. However, I hope that in the future sampleset creators (and users that modify odfs) won't place such things in the ODF anymore - I for one sure as hell won't! (the main reason for me is that there are already default values that makes it un-necessary to write such things at all, and all the default adaptations I want to make will be shipped with a bundled .cmb instead that's so much nicer to work with)

The messages you suggested won't be clear in this case.

In case you didn't know already - you won't get the log message if the ChurchName is the same! You will however get the message box you mentioned at start of this issue. I plan to change that message partly according to your suggestion, but since it's not necessarily only combinations that are imported I think that the message box text could be: The .cmb file does not exactly match the current ODF. Importing it can cause various problems. Should it really be imported? What do you think about that?

@oleg68
Copy link
Contributor Author

oleg68 commented Oct 29, 2021

For such changes the current .cmb import won't work

I import Combinations, not Organ Settings. And now it works exept the extra error message.

@larspalo
Copy link
Contributor

I import Combinations, not Organ Settings. And now it works exept the extra error message.

It's done the same way anyway in GO... It's just that the settings in the .cmb should be ignored if you only import the combinations. And then no reading of any PitchTuning lines will be done from the .cmb either of course. And you should not have any error message, right? It's only a message box asking for a confirmation of the import, right?

@oleg68
Copy link
Contributor Author

oleg68 commented Oct 29, 2021

@larspalo Please don't make a message box, because it requires an extra click. A warning in the message log is enough.

@larspalo
Copy link
Contributor

No, ok. You're right, there's a different LoadCombination! It basically behaves the same though in the background. I'll make a separate PR for this then!

@larspalo
Copy link
Contributor

Right, so for only combination import, you think it's ok with the current limitation of needing same ChurchName?

@larspalo
Copy link
Contributor

And you just want a log message with the text you mentioned above?

@oleg68
Copy link
Contributor Author

oleg68 commented Oct 29, 2021

And you just want a log message with the text you mentioned above?

Yes, I do.

@larspalo
Copy link
Contributor

Ok, coming soon!

@oleg68
Copy link
Contributor Author

oleg68 commented Oct 29, 2021

Right, so for only combination import, you think it's ok with the current limitation of needing same ChurchName?

I do not like limitations at all. If you make the same message box with changing ChurchName as for importing organ settings it will be perfect.

@larspalo
Copy link
Contributor

Ok, I'll change that too then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants