Renaming HtmlFormat to ToolTipFormat, and creating a new HtmlFormat. #343
Conversation
…ting from the old HtmlFormat is the same formatting used in the new ToolTipFormat. Everything that was using the old HtmlFormat is now using ToolTipFormat. Adding a new HtmlFormat that currently creates the same table as the ToolTipFormat, but without the yellow background color. The new HtmlFormat can be further refined later if it needs to be.
|
This was done in an effort to re-use the getInfoString method to be able to display information about data container geometries in IMFViewer. Currently before this PR, the HtmlFormat formatting looks super odd because it is mainly used for tooltips and has a pale yellow background. This PR addresses that and creates a basic HTML format with a blank background that applications can use. Mike's main concern is that the ToolTipFormat uses HTML also, so developers who want to use this API may get confused as to which one to use. I am open to hearing any better ways of designing this so that anything can ask a data container's geometry for its information string and get a usable string in return that looks nice. |
| case SIMPL::ToolTipFormat: | ||
| bgColor = "bgcolor=\"#FFFCEA\""; | ||
| nameBgColor = "bgcolor=\"#E9E7D6\""; | ||
| case SIMPL::HtmlFormat: |
There was a problem hiding this comment.
Add a comment here (before the case SIMPL::HtmlFormat:) stating that the fall-through is intentional. Better yet, use a commented out [[fallthrough]] attribute to document the intention. Without any kind of documentation of intent, this looks like a mistake and could be easily "fixed" while trying to reduce compiler warnings.
| case SIMPL::ToolTipFormat: | ||
| bgColor = "bgcolor=\"#FFFCEA\""; | ||
| nameBgColor = "bgcolor=\"#E9E7D6\""; | ||
| case SIMPL::HtmlFormat: |
There was a problem hiding this comment.
If fall-through is intentional, make sure to document it ever time. // [[falthrough]] should be good enough while remaining short and easy to read. We can't use the fallthrough attribute as it's C++17, but it's everything we would want to use while remaining far more easily readable than a longer comment stating that we want to use [[fallthrough]]
| ss << "<tr " << bgColorLabel << "><th align=\"right\">Extents: </th><td>" | ||
| << "<p>" << xExtentString << "</p>" | ||
| << "<p>" << yExtentString << "</p>" | ||
| << "<p>" << zExtentString << "</p>" |
There was a problem hiding this comment.
This seems weird like some of these lines are supposed to be different than the others because half of them are treated the same with ss at the start and half do not. Try to keep them all consistent, or it looks like they are not supposed to be the same without any decipherable reason to their difference.
There was a problem hiding this comment.
Make sure you have some kind of documentation for every case intentionally involving fall-through between cases so that they do not look like coding errors and get "fixed" after compiler warnings label them as such. You did so rather consistently, so I only marked the first two.
Make sure you consistently use ss << ... and << ... because mixing and matching makes it look like there is supposed to be a difference. ss << ... is the most common, so use that. It looks far cleaner and is easier to read when it's formatted the same, especially when no one starts reading through them to find out why some of them are treated as special cases. At least one person already has.
imikejackson
force-pushed
the
develop
branch
from
4998629
to
1d8ba0b
Compare
Replacing the current HtmlFormat with a new ToolTipFormat. The formatting from the old HtmlFormat is the same formatting used in the new ToolTipFormat. Everything that was using the old HtmlFormat is now using ToolTipFormat.
Adding a new HtmlFormat that currently creates the same table as the ToolTipFormat, but without the yellow background color. The new HtmlFormat can be further refined later if it needs to be.