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

Behind-the-scenes code tidying for Json data handling #396

Merged
merged 3 commits into from
Feb 27, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Dec 27, 2019

This PR makes several sweeping (but fairly minor) changes to the way Json data-passing is implemented across the codebase:

  1. Parsing from std::string to Json::Value is now done by a new utility function openshot::stringToJson(), which in turn lives in a new source file src/Json.cpp. All SetJson() methods in the library now call that function instead of repeating the same parsing boilerplate everywhere.
    • openshot::stringToJson() is now the only place in the src/ directory where the Json::Value::parse() method is called, and the only place where the code builds a Json::CharReader object from Json::CharReaderBuilder. (There's one method in src/ChunkReader.cpp which parses an input file stream using a Json::CharReaderBuilder with Json::parseFromStream().)
    • The "in the src/ directory" qualifier above is because tests/Clip_tests.cpp still does its own parsing, rather than using openshot::stringToJson(), for sanity-checking purposes.
    • This greatly reduces copy-paste code duplication in the SetJson() methods, though not as much as I'd like. (It really feels like it should be possible to template them away completely, to be honest.)
  2. Use of const member functions and args is greatly expanded where appropriate.
  3. Code that was using std::stringstream solely to format numeric values as strings now does so directly, with C++11's std::to_string().
  4. Docstrings across the codebase referred to the nonexistent type Json::JsonValue, which has been corrected to Json::Value.

Abstract base class changes

Item 2 on the above list applies to (some of) the abstract base classes as well, like ClipBase and ReaderBase. Because the pure virtual class methods Json() and JsonValue() of the abstract base classes were changed to have a const signature, their derived classes have to declare the concrete overrides for those methods const as well or the signatures won't match.

This doesn't really affect users of the code (I simply changed all of the relevant declarations in the derived classes as well), but it will impact any library consumers if they declare their own classes derived from the base classes. For instance, the only change I had to make to any of the unit test files was in tests/ReaderBase_tests.cpp:

diff --git a/tests/ReaderBase_Tests.cpp b/tests/ReaderBase_Tests.cpp
index 8ac2832..e3062bc 100644
--- a/tests/ReaderBase_Tests.cpp
+++ b/tests/ReaderBase_Tests.cpp
@@ -44,19 +44,19 @@ TEST(ReaderBase_Derived_Class)
 	class TestReader : public ReaderBase
 	{
 	public:
 		TestReader() { };
 		CacheBase* GetCache() { return NULL; };
 		std::shared_ptr<Frame> GetFrame(int64_t number) { std::shared_ptr<Frame> f(new Frame()); return f; }
 		void Close() { };
 		void Open() { };
-		string Json() { return NULL; };
+		string Json() const { return NULL; };
 		void SetJson(string value) { };
-		Json::Value JsonValue() { return (int) NULL; };
+		Json::Value JsonValue() const { return (int) NULL; };
 		void SetJsonValue(Json::Value root) { };
 		bool IsOpen() { return true; };
 		string Name() { return "TestReader"; };
 	};
 
 	// Create an instance of the derived class
 	TestReader t1;

Without that change, the ReaderBase_Derived_Class test fails to compile, because the TestReader class fails to implement the pure virtual const methods in ReaderBase.

Like I said, this won't affect OpenShot, or normal consumers of the library, but it might affect @DylanC, @jeffski, or other users of libopenshot if they declare their own classes derived from our abstract base classes. That's kind of unfortunate, and if anyone knows a good way around that, so that derived classes will continue to compile without changes despite these changes to the base class definitions, I'd love to incorporate a fix.

- Parsing from string to Json::Value is now done by utility function
  openshot::stringToJson() in Json.cpp, all SetJson() methods call it.
- Expand use of const member functions and args where appropriate.
- Use std::to_string() to format int/float values as strings.
- Correct mentions of nonexistent Json::JsonValue type in docstrings
@ferdnyc ferdnyc added the code Source code cleanup, streamlining, or style tweaks label Dec 27, 2019
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 27, 2019

Today's lesson: Code builds much better when you remember to commit the new file.

Also, it seems the FFmpeg PPA "walkout" has ended, as our FFmpeg4 build jobs are passing again.

@jonoomph
Copy link
Member

This is a big PR, but I have no problem with any of the changes.

@jonoomph jonoomph merged commit 4f591c7 into OpenShot:develop Feb 27, 2020
@ferdnyc ferdnyc deleted the json-parsing branch May 19, 2020 03:33
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants