traverso-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Traverso-devel] "Audio Import Option" second patch


From: Remon | Traverso-DAW
Subject: Re: [Traverso-devel] "Audio Import Option" second patch
Date: Sat, 8 Jun 2019 13:50:44 +0200

Hello Shi,

Good work on the import patch, here some general guidelines and some ideas.

1. Always use getter and setter functions to change private/protected member variables
This way we have much better control and knowledge about execution flow and we're able to debug much easier. It is also more future proof.

2. function naming
A bit nitpicking perhaps, but we use always lower_case_function_names() and not upper_Case_Function_Names(). Just for better readability

3. Isolation functionality and future proofing code
I think AudioFileCopyConvert is the beating hearth of the import code when it comes to threaded copy/samplerate conversion or even converting ogg/mp3 file to for example wavpack or wav
My idea was to create a TImportWidget that also has a section:
convert audiofiles [ checkbox ]
combobox file type (wav, wavpack, flac, ogg)
combobox samplerate (audiodevice sample rate, 44.1 kzh 48Khz etc)

Now use above parameters to inform the ReadSource and ExportSpec in AudioFileCopyConvert to read the audio data with the correct sample rate and write using the correct file format

Other options would be adding at single track, splitting stereo to mono etc etc

The import/copy audio fies in NewProjectDialog seems like a logic place to start from, move the code that adds the files to the view and the up/down logic to TImportWidget
Perhaps use a tabbed widget approach or wizzard like style to avoid cluttering too many options in one Dialog page

Hope this makes any sense

4. Behavior change
Previously an import of multiple files was grouped into a CommandGroup object I think so all files could be removed/readded by one un/redo. Now everything is a single command on the Historystack, my feeling is that it makes more sense to keep the behavior the old way?

5. TImportWidget at wrong location
Also nitpicking but at first I could not find the file where I expected it to be: src/traverso/widgets, it is now placed in src/traverso/dialogs/project/

6. TImportDialog::set_dialog_dragdrop_type_or_not(bool flag)
Sometimes I use this kind of function but then I realize how hard it is to understand what the heck are we actually doing here?
perhaps better to name it:
set_dialog_is_a_drag_drop();
or use an enum or something:
set_dialog_import_type(int type);
idialog().set_dragdrop_type(TImpoartDialog::TypeDragDrop);

7. idialog()
Another example where short naming is probably not the best idea, used it too much myself actually.
When reading the code in say over one year, what dialog are we referring too ?
QtCreator has excellent code completion so better to have more meaningful naming like;
audioImportDialog()

ied() is such an example where even I have a difficult time to remember that it means inputEventDispatcher()

8. Not sure if there is a difference when importing audio from different point in the workflow
In other words, Import by  < I > or import audio by D&D or at Project creation, should we differentiate?
I noticed in your patch that D&D is considered a specific case but I'm not sure if we need to do that ?
If you like to elaborate on that...

Keep up the good work!

Regards,
Remon

Op vr 7 jun. 2019 om 12:28 schreef 石任之 <address@hidden>:
Hello, This is the second patch :)
Most of logic done.

This implementation have two main issue:
1 The result of samplerate conversion is not correct if enable "copy files".
2 When importing files using drag&drop method, start position not always correct.
_______________________________________________
Traverso-devel mailing list
address@hidden
https://lists.nongnu.org/mailman/listinfo/traverso-devel

reply via email to

[Prev in Thread] Current Thread [Next in Thread]