Add 16kHz sample rate and fix 91kHz CB label. #2092
Conversation
This commit implements the change suggested in xournalpp#2091. It also changes the unmatched combo box label value "91000" to "96100" to reflect the integer value that the `load` settings function expects.
rolandlo
left a comment
There was a problem hiding this comment.
@step-
Thanks for your PR. Changing the default sample rate will come as a surprise to those who have used the audio recording before with the previous default. It would be better to keep 44100 Hz as the default. If someone runs into trouble (like you did) with this setting he will most likely check play around with the settings anyway and find that 16000 Hz works for him.
So unless you convince me of the opposite I would ask you to change back the default to 44100 Hz. The rest of the PR looks fine to me.
|
@rolandlo I was under the impression that the default value is saved to the user's configuration file on first run, therefore existing users wouldn't be affected by the new default because they already have a configuration file. But it doesn't matter what the default value is, as long as 16 kHz is offered as a choice. I committed the change you requested. |
|
@step- Ok, you are right about that. It only occurs after deleting the settings file. Anyway 44100 Hz is a very commonly used sample rate. You have changed back the default in the Settings.cpp file, but not in the SettingsDialog.cpp file. So please change that as well. Looking at various sources about sample rates (e.g. wikipedia) I find that 16000 Hz, 44100 Hz and 19200 Hz are all commonly used sample rates, but 96100 Hz is not. It should be 96000 Hz instead. So you should change the number 96100 to 96000 (consistently). |
|
@rolandlo, yeah, I was wondering too why the original code used 96.1kHz instead of the more common 96kHz but since I found some internet sources citing 96.1kHz I didn't raise any question. Anyway, I've implemented the changes you requested. |
|
Ok, everything works fine now. Can you please squash your commits into one (following this tutorial)? Alternatively I can do it for you from within github. After the squashing I will merge your PR. |
This commit implements the change suggested in xournalpp#2091 adding 16 kHz audio sample rate (the default rate remains 44 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000.
…/github.com/step-/xournalpp into 16kHz-sample-rate+fix-91kHz-combo-box-label
Fix item id is set inconsequently for the 96000 Hz sample rate. Co-authored-by: Roland Lötscher <40485433+rolandlo@users.noreply.github.com>
This commit implements the change suggested in xournalpp#2091 adding 16 kHz audio sample rate (the default rate remains 44 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000.
This commit implements the change suggested in xournalpp#2091 adding 16 kHz audio sample rate (the default rate remains 44 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000.
|
@step- Thanks for your commit. I have merged it. |
Add 16 kHz sample rate and sundry. This commit implements the change suggested in xournalpp#2091 adding 16 kHz audio sample rate (the default rate remains 44.1 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000.
Add 16 kHz sample rate and sundry. This commit implements the change suggested in xournalpp#2091 adding 16 kHz audio sample rate (the default rate remains 44.1 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000. Backport: e2a0f1d
Add 16 kHz sample rate and sundry. This commit implements the change suggested in #2091 adding 16 kHz audio sample rate (the default rate remains 44.1 kHz). This commit also changes the 96.1 kHz rate to 96 kHz, which is more commonly used, and fixes the unmatched combobox label 91000. Backport: e2a0f1d
This commit implements the change suggested in #2091. It also changes
the unmatched combo box label value "91000" to "96100" to reflect the
integer value that the
loadsettings function expects.