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

Documentation comment fixes #642

Merged
merged 8 commits into from Apr 1, 2021
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Mar 7, 2021

This PR fixes a few widespread issues with the documentation comments in our header files, that affect the Doxygen-generated API docs. The issues fixed are:

Using both documentation comment blocks and member comments together

Any given item (function, variable, etc.) can only have one documentation comment associated with it. This (though previously very common in our code) does not work:

/// Get and Set JSON methods
void SetJson(const std::string value); ///< Load JSON string into this object
void SetJsonValue(const Json::Value root); ///< Load Json::Value into this object

It will result in the documentation for SetJson being "Get and Set JSON methods", as the preceding-comment-block documentation will always take priority.

(Technically member documentation — the trailing ///< ... type — isn't supposed to be used for functions at all, only for variables and members of structs and enums. I think it only works here because class methods are technically members of a struct. But it'll still break if there's a preceding doc comment.)

Placing documentation comments over groups of functions

Documentation comments can only be used to document a single feature (class, function, variable, etc.). Placing a documentation comment over multiple functions doesn't make sense:

/// Internal blur methods (inspired and credited to http://blog.ivank.net/fastest-gaussian-blur.html)
void boxBlurH(unsigned char *scl, unsigned char *tcl, int w, int h, int r);
void boxBlurT(unsigned char *scl, unsigned char *tcl, int w, int h, int r);

This will result in boxBlurH having the comment text as its documentation, and boxBlurT being undocumented.

Constructor terminology

Finally, a terminology note. A default constructor is one that can be called with no arguments. Full stop. That is the definition. A few of our headers contained documentation comments like this, which are just wrong and confusing:

/// Blank constructor, useful when using Json to load the effect properties
Pixelate();

/// Default constructor, which takes 5 curves. These curves animate the pixelization effect over time.
Pixelate(Keyframe pixelization, Keyframe left, Keyframe top, Keyframe right, Keyframe bottom);

So, I tried to correct as many of those as I could, to label the correct constructor as the default.
That abuse of terminology so threw poor @BrennoCaldato that the OpenCV effect headers ended up with comments like these:

/// Blank constructor, useful when using Json to load the effect properties
Tracker(std::string clipTrackerDataPath);

/// Default constructor
Tracker();

Which... I mean, the second comment is correct so it's hard to fault it. Still, I just removed those comments entirely because they didn't provide any API details that required documentation.

Hopefully I fixed all of these, I tried to find them all but I can't promise I didn't miss any.

Beyond those issues I also fixed some nested #ifdef nonsense I created in deprecating TooManySeeks, and polished up some other minor issues.

@ferdnyc ferdnyc added docs Bad or missing help texts / documentation code Source code cleanup, streamlining, or style tweaks labels Mar 7, 2021
@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #642 (9be8e29) into develop (3f9b402) will increase coverage by 0.01%.
The diff coverage is 20.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #642      +/-   ##
===========================================
+ Coverage    51.74%   51.75%   +0.01%     
===========================================
  Files          151      151              
  Lines        12254    12255       +1     
===========================================
+ Hits          6341     6343       +2     
+ Misses        5913     5912       -1     
Impacted Files Coverage Δ
src/CVObjectDetection.h 0.00% <ø> (ø)
src/CVStabilization.h 100.00% <ø> (ø)
src/CVTracker.h 81.81% <ø> (ø)
src/CacheBase.h 75.00% <ø> (ø)
src/CacheMemory.h 0.00% <ø> (ø)
src/ChunkReader.h 0.00% <ø> (ø)
src/Clip.h 75.00% <ø> (ø)
src/ClipBase.h 80.76% <ø> (ø)
src/Color.h 100.00% <ø> (ø)
src/CrashHandler.h 100.00% <ø> (ø)
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f9b402...9be8e29. Read the comment docs.

Codecov's bash script displays an error if the checkout action
uses its default depth of 1 for the checkout, so we need to
explicitly set it to 0 (or some number > 1).
@github-actions github-actions bot added the conflicts A PR with unresolved merge conflicts label Mar 31, 2021
@github-actions
Copy link

Merge conflicts have been detected on this PR, please resolve.

@ferdnyc ferdnyc removed the conflicts A PR with unresolved merge conflicts label Apr 1, 2021
@ferdnyc ferdnyc merged commit b232919 into OpenShot:develop Apr 1, 2021
@ferdnyc ferdnyc deleted the fix-doc-comments branch April 1, 2021 00:41
@ferdnyc ferdnyc mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks docs Bad or missing help texts / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant