tinfoilboy / ctml Goto Github PK

Back when I wrote the code for this library, I had this habit where everything was mashed together and there were comments strewn all around.

I feel that this is harder to read now, and in need of change, as anyone else making changes/using the library would most likely find it's source cryptic.

When running the tests, one may get (at least on my machine with GCC 8.2)

No Close Test failed!
Node Output: <img alt="Hilarious image" src="funnypicture.png">
Expected Output: <img src="funnypicture.png" alt="Hilarious image">

Because of this

I'm not sure, whether someone actually wants the attributes serialized in the same order they were defined. Otherwise, the unit tests needs to be adjusted.

Pls correct me if I'm wrong but I think that the following line should be checking for content instead of m_content

Getting children from a Node currently has to be done by either iterating the children or knowing the index of the child that you wish to grab. It may be beneficial to add functionality to grab these children via the selector parsing that is already in the library for creating elements from abbreviations. This would then allow for more flexibility in modifying existing structures and closer mimic the methods used for getting elements in the browser.

There are some HTML tags, which do not have a closing tag. For example the <meta> tag below is valid:

<meta charset="UTF-8">

While CTML

CTML::Node{"meta"}
  .SetAttribute("charset", "UTF-8")
  .ToString(CTML::Readability::SINGLE_LINE)

outputs

<meta charset="UTF-8"></meta>

This is invalid HTML and parsers do not need to deal with it correctly.

First, thank you for the clean rewrite of 1.0.0. This broke everything in my code. ;D

I have now another issue. I'm using 0b8ba53 (currently latest) commit.

While

CTML::Node tableRow{"tr"};
tableRow.AppendChild(CTML::Node("td", "my_content1"));

works, the following doesn't work anymore:

CTML::Node tableRow{"tr"};
tableRow.AppendChild(CTML::Node("td").SetContent("my_content2"));

In the first example my_content1 is the content, like expected, while in the second the call of SetContent will not set the string as content.

It's not actually a problem, since the first code does the job perfect. But maybe some legacy code will break.

I want to construct an HTML doc, display it, and update it as the user does things. At present there's not way to delete nodes, only add them.

How can I append a text to a node?

For example:

CTML::Node par{"p", "Hello world "};
...
par.AppendText("text");

Or, how can I create this node?

<p>Hello world. <span>Hello</span> You</p>

I made new node types to support these and updated the Node() constructor and ToString() function to support them. It's not perfect and it's very quick and dirty but it added the missing functionality:

This allows images to be used as:
CTML::Node(CTML::NodeType::IMG, "", "img\\logo.png");

Node(
	const NodeType& type,
	std::string name="",
	std::string content=""
)
.
.
.
else if (type == NodeType::IMG
	|| type == NodeType::HREF
	|| type == NodeType::LINK)
{
	this->SetName(name);
	m_content = content;
}

std::string ToString(ToStringOptions options={}) const
.
.
.
else if (m_type == NodeType::IMG)
{
	output << indent << "<img " ;
	output << "src=" << m_content << ">";
}
else if (m_type == NodeType::HREF)
{
	output << indent << "<a href=" << m_content << ">" << m_name << "</a>";
}
else if (m_type == NodeType::LINK)
{
	output << indent << "<link " << m_content << "/>";
}

A few possible enhancements:

There are extra copies of Node objects in code like this:

                Node& AppendChild(Node child) {
			m_children.push_back(child);
			return *this;
		}

This would be more efficient like:

                template<class T>
                Node& AppendChild(T&& child) {
			m_children.push_back( std::move(child) );
			return *this;
		}

Likewise _CountOccurrences(std::string finder, const std::string& findable) and _ParseClassesAndIDS(std::string classesAndIDs) appear to be able to take a const ref to string to avoid a copy.

Depending on which C++ standard you are supporting, you could also accept std::string_view instead of std::string in your interface, which may avoid copies.

I also notice you have used names that are reserved for the implementation when you name things with an underscore and capital letter like _GetFormattedContent, which means it's technically undefined behavior.

The <html> tag accepts global attributes. Currently, this is not possible to add such attributes (e.g. lang) as the <html> tag is hard coded and not an actual CTML::Node.

It would be a bit nicer to be able to parse a string and get a list of nodes from that string for uses such as creating nodes from a string and appending that to a document, or even loading an html file and being able to change the content of it for templating or similar.

I feel like this could be done with a simple function like

std::vector<CTML::Node> parse_nodes_to_string(const std::string& content)

Which then would just return the list of nodes that it was able to parse.

Hello I like the idea of the library and it's pretty intuitive to use but i notices a few thing about it that doesn't seem right.

#include "CTML.h"
#include <iostream>

int main ()
{
    const CTML::Document document;
    std::cout << document.ToString(CTML::MULTILINE);
}

This will fail to compile due to ToString not being a const member function. Now unless ToString mutates Document it should be const function.

I also noticed this with your get functions have the same issue.

It would be nice to be able to add to a footer as AppendNodeToHeader and AppendNodeToBody work.