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

issue #7231 Improvement of layout of refines relations. #7236

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

albert-github
Copy link
Contributor

Implemented:

  • no extra "Refines" page
  • Refines on one line
  • Refines always in code style
  • 2 arguments -> two arguments (for consistency)
  • added also the \qualifier

Implemented in Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h Results a.o.

  • doc_output/Kernel_23/classKernel_1_1CartesianConstIterator__2.html
    image

  • doc_output/Kernel_23/classKernel_1_1BoundedSide__3.html
    image

Note: due to the fact that this is a test implementation pages outside the scope of Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h might not be correct (in respect to code appearance of refines and multiple refines)

Implemented:
- no extra "Refines" page
- Refines on one line
- Refines always in code style
- `2 arguments` -> `two arguments`  (for consistency
- added also the `\qualifier`

Implemented in  Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h
Results a.o.
- doc_output/Kernel_23/classKernel_1_1Circle__2.html
- doc_output/Kernel_23/classKernel_1_1BoundedSide__3.html

Note: due to the fact that this is a test implementation pages outside the scope of Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h might not be correct (in respect to code appearance of refines and multiple refines)
Updating the 1.8.4 and 1.8.13 version as well
@albert-github

This comment was marked as duplicate.

1 similar comment
@lrineau

This comment was marked as duplicate.

@github-actions

This comment was marked as duplicate.

1 similar comment
@github-actions
Copy link

github-actions bot commented Feb 2, 2023

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/7236/v0/Manual/index.html

@afabri
Copy link
Member

afabri commented Feb 2, 2023

For me a title is a title, and it looks cluttered with the additional boxes.

@albert-github
Copy link
Contributor Author

@afabri
Thank you for the valuable feedback, this can help in deciding what should be exactly done as it was written by @sloriot in #7231 (comment): "We should start by modifying one page, look at the output and agree on that change before applying it globally."

Hopefully more comments will follow so we can make a good decision soon (and apply it on other places as well).

@MaelRL
Copy link
Member

MaelRL commented Feb 2, 2023

  • I think the "Concept / Class Reference" near the actual concept / class name is just visual noise
  • I would remove the "AdaptableFunctor ..." box near "Class Reference", which doesn't add anything
  • The contrast between bold "refines" followed by colon and a one-line list, and then bold "see also" that does the exact opposite is not very nice.

Why not use the same the same presentation as "Inherits" (e.g. https://doc.cgal.org/latest/Triangulation_3/classCGAL_1_1Regular__triangulation__3.html)?

In fact, I would actually inherit concepts so all the requirements of a single concept would show on a single page, and you don't have to walk Periodic_3MeshTriangulation_3 > MeshTriangulationTraits_3 > RegularTriangulationTraits_3 > TriangulationTraits_3 > SpatialSortingTraits_3 to see if something is already documented somewhere in the stack.

@albert-github
Copy link
Contributor Author

@MaelRL Thanks for the reaction.

  • I think the "Concept / Class Reference" near the actual concept / class name is just visual noise

    I assume you mean here the "Class Reference" e.g. here:
    image

    The problem here is that this is automatically added by doxygen and won't be possible to remove

  • I would remove the "AdaptableFunctor ..." box near "Class Reference", which doesn't add anything

    Supporting the comment of @afabri

  • The contrast between bold "refines" followed by colon and a one-line list, and then bold "see also" that does the exact opposite is not very nice.

    There are her 2 items mentioned

    • The layout of the "Refines". It is possible to make it like:
      image
      This requires a small change in the ALIASES to e.g.:
      "cgalRefines=<dl><dt><b>Refines</b>:</dt><dd></dd><dt></dt><dd>" \
      "cgalRefines{1}=@qualifier \"\1\" ^^ @cgalRefines  @c \1.</dd></dl>" \
      "cgalRefines{3}=@qualifier \"\1\" ^^ @qualifier \"\2\" ^^ @qualifier \"\3\" ^^ @cgalRefines @c \1, @c \2 and @c \3.</dd></dl>" \
      
    • The "See also", I think this is also one of the possible candidates to be written on one line.
      • it is possible to do this with the current \sa like:
          \sa `CGAL::Iso_cuboid_3<Kernel>`, `CGAL::Sphere_3<Kernel>` and `CGAL::Tetrahedron_3<Kernel>`
        
        so we get:
        image
        We can also make an ALIASES analogous the \cgalRefines... with name cgalSeeAlso though the problem with small problem with see also is that it can also refer to things like \ref (e.g. \sa Section \ref tds3cyclic) and \link ... \endlink so it has to be checked whether an "automatic" version is possible.
        As a side note: I see some inconsistent use where not all "class" names etc. (like in \sa CGAL::Exact_predicates_inexact_constructions_kernel) are embedded in back-ticks.
  • Why not use the same the same presentation as "Inherits"

    I think this would take a bit a lot of place and one of the purposes is to reduce the the space required.

  • In fact, I would actually inherit concepts so all the requirements of a single concept would show on a single page, and you don't have to walk Periodic_3MeshTriangulation_3 > MeshTriangulationTraits_3 > RegularTriangulationTraits_3 > TriangulationTraits_3 > SpatialSortingTraits_3 to see if something is already documented somewhere in the stack.

    Can you give a reference where this happens (page name) / give an example?

@MaelRL
Copy link
Member

MaelRL commented Feb 3, 2023

The problem here is that this is automatically added by doxygen and won't be possible to remove

That's a shame :)

I think this would take a bit a lot of place and one of the purposes is to reduce the the space required.

I'm not very sensible to this space argument: nobody is printing CGAL documentation and it becomes less readable on a single line. We already have "examples where this function/class appears" where it's on the same line, and I find it poor:

image

It will be the same if you put all "see also" and concepts with longer names on the same line.

Can you give a reference where this happens (page name) / give an example?

Do you mean a concept having many refinements? What I quoted is a real concept stack (https://doc.cgal.org/latest/Mesh_3/classMeshTriangulationTraits__3.html). If you meant, a page with refinement of concepts using inheritance, there is one here.

@albert-github
Copy link
Contributor Author

albert-github commented Feb 3, 2023

  • place argument, this is a arbitrary and user dependent. It also has to do a great deal with readability and getting a quick overview.
  • the "examples" indeed it is hard to see where the one example ends and the next example starts (but also user dependent.
  • "concept stack", here we see indeed that there are quite a few "refines on refines"
    MeshTriangulationTraits_3 > RegularTriangulationTraits_3 > TriangulationTraits_3 > SpatialSortingTraits_3
    I think it would indeed be nice to have this line (with the links) at the different levels. Maybe also a "Refined by" would be nice.
    I think though that this might be a bit out of scope for this issue / PR, but it is (in my opinion) certainly worthwhile to investigate whether or not these 2 things are possible.
    (Note the page https://cgal.github.io/6849/v2/Isosurfacing_3/classIsosurfacingDomainWithGradient.html is in the 1.8.13 version a complete disaster, is there a corresponding page generated with the current master version?)

@MaelRL
Copy link
Member

MaelRL commented Feb 3, 2023

place argument, this is a arbitrary and user dependent. It also has to do a great deal with readability and getting a quick overview.

We are apparently OK with the place that the "inherits" Section takes on class pages, and that is a lot more space than the "Refines" takes.

I think it would indeed be nice to have this line (with the links) at the different levels. Maybe also a "Refined by" would be nice.

If you don't include the actual requirements on the page, you don't remove the fact that you need to click away and try to remember what you saw at previous concept levels. If you see look at https://doc.cgal.org/latest/Triangulation_3/classCGAL_1_1Regular__triangulation__3.html for example, it shows:

image

which is it's base's base. And it makes sense, because if we only showed CGAL::Triangulation_3 (the base), people would get annoyed that you have to click to CGAL::Triangulation_3 to see the functions of the base's base. Yet this is what we do for concepts.

I think it would indeed be nice to have this line (with the links) at the different levels. Maybe also a "Refined by" would be nice.

Sure, like for class inheritance :>

I think though that this might be a bit out of scope for this issue / PR, but it is (in my opinion) certainly worthwhile to investigate whether or not these 2 things are possible.

Seems relevant to me because I'm arguing that we could fully harmonize presentation of "refinement" and presentation of "inheritance".

(Note the page cgal.github.io/6849/v2/Isosurfacing_3/classIsosurfacingDomainWithGradient.html is in the 1.8.13 version a complete disaster, is there a corresponding page generated with the current master version?)

No, but it did not use any fancy trick: you just need to put an actual inheritance in the concept, and doxygen does the rest

class Concept
  : public ConceptBase
{ }

@albert-github
Copy link
Contributor Author

albert-github commented Feb 3, 2023

Regarding the Note (and the mess in the 1.8.13 version), I'm not sure whether it is doxygen that creates the mess or that it is some postprocessing that is done by CGAL. Is there another place where this type of construct is used (Isosurfacing_3 is a new package and not in the "master" set present), I remember seeing it at other places as well but don't remember where.

@albert-github
Copy link
Contributor Author

@MaelRL Looks like I found the "disaster" type of documentation again through #695. Problem looks like to be only present in /build/v... type of builds ...

@albert-github
Copy link
Contributor Author

Are there any more comments / thoughts about this issue?

@lrineau lrineau linked an issue Feb 20, 2023 that may be closed by this pull request
@sloriot
Copy link
Member

sloriot commented Mar 13, 2023

So if I get it right, the conclusion is that people like the proposal except that we should not use \qualifier. Do you know if an official doxygen support for concept is in the work?

@sloriot
Copy link
Member

sloriot commented Mar 13, 2023

I mean we want:

  • no extra "Refines" page
  • Refines on one line
  • Refines always in code style
  • 2 arguments -> two arguments (for consistency)
  • added also the \qualifier

@albert-github albert-github marked this pull request as draft March 14, 2023 09:36
@albert-github
Copy link
Contributor Author

Do you know if an official doxygen support for concept is in the work?

I never worked with concepts so I cannot really tell about the support and how it works, but in the doxygen 1.9.2 version there is the feature (see the doxygen changelog ):
issue #2732: Adding support for C++20 concepts (Origin: bugzilla #499352) [view], [view], [view]

Regarding the summary in #7236 (comment) I'm going to work on it (probably this or next week).

@albert-github albert-github marked this pull request as ready for review March 14, 2023 16:38
@albert-github
Copy link
Contributor Author

Any comments or plans for integration?

@sloriot
Copy link
Member

sloriot commented Apr 6, 2023

I just missed that it was ready for testing. I'll test it in the next batch

@sloriot sloriot modified the milestones: 5.6, 5.6-beta Apr 6, 2023
@sloriot sloriot added the Batch_2 Second Batch of PRs under testing label Apr 7, 2023
@albert-github
Copy link
Contributor Author

@sloriot Thanks for the review, I posted a number of questions before I start updating. Some of the suggested changes I have to look into.
I see that the 4 / 5 Adaptable functor problem already has been addressed, that is nice and quick. Thanks I can start updating.

@albert-github
Copy link
Contributor Author

@sloriot

In Voronoi_diagram_2/doc/Voronoi_diagram_2/Concepts/AdaptationTraits_2.h I found:

A type for a functor that accesses the
site associated with a vertex. The functor should be a model of the
concepts `DefaultConstructible`, `CopyConstructible`,
`Assignable` and `AdaptableFunctor` (with one argument).

Should here also the `AdaptableFunctor` (with one argument) be replaced with `AdaptableUnaryFunction` ?

@sloriot
Copy link
Member

sloriot commented Apr 11, 2023

@sloriot

In Voronoi_diagram_2/doc/Voronoi_diagram_2/Concepts/AdaptationTraits_2.h I found:

A type for a functor that accesses the
site associated with a vertex. The functor should be a model of the
concepts `DefaultConstructible`, `CopyConstructible`,
`Assignable` and `AdaptableFunctor` (with one argument).

Should here also the `AdaptableFunctor` (with one argument) be replaced with `AdaptableUnaryFunction` ?

Exactly

@albert-github
Copy link
Contributor Author

Any suggestion for:

  \cgalRefines{AdaptableFunctor (with two or three arguments)}

as found in Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h

In the same file there are a number of occurences of:

\cgalRefines{AdaptableFunctor}

what to do wit these?

Furthermore there are the occurrences:

Algebraic_foundations/doc/Algebraic_foundations/Concepts/AlgebraicStructureTraits--DivMod.h:\cgalRefines{AdaptableFunctor}
Algebraic_foundations/doc/Algebraic_foundations/Concepts/AlgebraicStructureTraits--RootOf.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_1--ConstructAlgebraicReal_1.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_2--ConstructAlgebraicReal_2.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_2--Isolate_2.h:\cgalRefines{AdaptableFunctor}
Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrTraits--CompareXOnBoundaryOfCurveEnd_2.h: * \cgalRefines{AdaptableFunctor}
Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrTraits--CompareXOnBoundary_2.h: * \cgalRefines{AdaptableFunctor}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--ConstructPolynomial.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--EvaluateHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--IsZeroAt.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--IsZeroAtHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Move.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Permute.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--PseudoDivision.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--ScaleHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--SignAt.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--SignAtHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Swap.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--TranslateHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}

How to handle these?

@sloriot
Copy link
Member

sloriot commented Apr 12, 2023

Any suggestion for:

  \cgalRefines{AdaptableFunctor (with two or three arguments)}

as found in Kernel_23/doc/Kernel_23/Concepts/FunctionObjectConcepts.h

In the same file there are a number of occurences of:

\cgalRefines{AdaptableFunctor}

what to do wit these?

Furthermore there are the occurrences:

Algebraic_foundations/doc/Algebraic_foundations/Concepts/AlgebraicStructureTraits--DivMod.h:\cgalRefines{AdaptableFunctor}
Algebraic_foundations/doc/Algebraic_foundations/Concepts/AlgebraicStructureTraits--RootOf.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_1--ConstructAlgebraicReal_1.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_2--ConstructAlgebraicReal_2.h:\cgalRefines{AdaptableFunctor}
Algebraic_kernel_d/doc/Algebraic_kernel_d/Concepts/AlgebraicKernel_d_2--Isolate_2.h:\cgalRefines{AdaptableFunctor}
Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrTraits--CompareXOnBoundaryOfCurveEnd_2.h: * \cgalRefines{AdaptableFunctor}
Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrTraits--CompareXOnBoundary_2.h: * \cgalRefines{AdaptableFunctor}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--ConstructPolynomial.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--EvaluateHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--IsZeroAt.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--IsZeroAtHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Move.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Permute.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--PseudoDivision.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--ScaleHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--SignAt.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--SignAtHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--Swap.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}
Polynomial/doc/Polynomial/Concepts/PolynomialTraits_d--TranslateHomogeneous.h:\cgalRefines{AdaptableFunctor,CopyConstructible,DefaultConstructible}

How to handle these?

\cgalRefines{AdaptableBinaryFunctor, AdaptableTernaryFunctor}

@afabri
Copy link
Member

afabri commented Apr 12, 2023

Honestly when we leave AdaptableFunctor without unary, binary, etc this is ok and not a mistake. Especially if it has various combinations of arity.

Adjusted after review
- usage of `Adaptable...Function` instead of `AdapatableFunctor (with... arguments)`
- corrected some incorrect / superfluous `}`
@albert-github
Copy link
Contributor Author

Corrected all mentions in the review above, ready for testing.

@sloriot sloriot added Under Testing and removed Batch_2 Second Batch of PRs under testing labels Apr 12, 2023
@sloriot
Copy link
Member

sloriot commented Apr 16, 2023

Successfully tested in CGAL-5.6-Ic-223

@lrineau lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Apr 17, 2023
@lrineau lrineau merged commit 824976a into CGAL:master Apr 17, 2023
@lrineau lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Apr 17, 2023
@lrineau lrineau deleted the feature/issue_7231 branch April 17, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement of layout of refines relations.
5 participants