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

Additional documentation regarding custom properties for QgsLayerTreeNode, QgsMapLayer, QgsLayoutAtlas, and QgsLayout #60904

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Alex-Kent
Copy link
Contributor

@Alex-Kent Alex-Kent commented Mar 9, 2025

Description

This PR expands the documentation of custom properties for the QgsLayerTreeNode, QgsMapLayer, QgsLayoutAtlas, and QgsLayout classes. In specific it lists all custom properties currently being used by QGIS with those classes (the classes listed are all of the ones that QGIS sets custom properties on). Additional custom properties may be [are] used by plugins; these have not been included.

Only comments are modified; there are no changes to any code.

The list of custom properties being used for QgsLayerTreeNode QgsMapLayer, QgsLayoutAtlas, and QgsLayout is current as of 9a1d453 (2025-03-06).

Comments

The impetus for determining which custom properties currently exist (at least those set by QGIS itself) was this comment on a recent PR.

Perhaps it would be useful to have a central registry of custom properties that are in use by QGIS and by plugins. This would be intended solely for use by developers. Such a registry could be as simple as including the property name, its function, and the class or plugin that uses it in the header file comments for QgsLayerTreeNode QgsMapLayer, QgsLayoutAtlas, and QgsLayout (as is partially done now for QgsLayerTreeNode).

@Alex-Kent Alex-Kent changed the title Additional documentation regarding custom properties for QgsLayerTreeNode and QgsMapLayer Additional documentation regarding custom properties for QgsLayerTreeNode, QgsMapLayer, QgsLayerTreeNode, and QgsMapLayer Mar 9, 2025
@Alex-Kent Alex-Kent changed the title Additional documentation regarding custom properties for QgsLayerTreeNode, QgsMapLayer, QgsLayerTreeNode, and QgsMapLayer Additional documentation regarding custom properties for QgsLayerTreeNode, QgsMapLayer, QgsLayoutAtlas, and QgsLayout Mar 9, 2025
* indicated); plugins may set additional ones:
*
* - "cached_name"
* QgsLegendModel::data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't usually document things like this. There's a couple of reasons why:

  • it goes out of sync very quickly, and ends up being more misleading then useful
  • a lot of these methods are private methods, and are not even accessible to users of the QGIS API
  • it's seen as a internal implementation detail only, not something which the user of the API needs to be aware of

Copy link
Contributor Author

@Alex-Kent Alex-Kent Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with some of what you said but I think there are good reasons to have these documented.

  • it goes out of sync very quickly

    As these properties are written to every project file one would expect the existing ones to remain valid in perpetuity. New properties might be added in future which are not included in this list and I agree there's no good way to handle that case. See also the following:

  • a lot of these methods are private methods

    These may be written/read by private methods but the resulting project files are text files that a user might want to parse or edit. (I've personally done both.)

    Ideally all properties would have a description of their function in the header file (as is currently done for some properties in QgsLayerTreeNode). Listing the methods where the values are set is a temporary measure and meant primarily as an aid to writing a proper description [currently the only way to determine which properties exist and their function is by manually reading through the source]; knowing what code writes each property makes it easier to determine precisely what values are being stored. Once descriptions have been written for each property then the lists in this PR that map property names to the methods that set them could be removed.

  • it's seen as a internal implementation detail only, not something which the user of the API needs to be aware of

    I disagree; custom properties are written to the project files and having their meanings documented is a good thing.

As I mentioned above, these lists are meant as a first step in writing proper documentation for the various custom properties that are in use. As an example, the first property listed for QMapLayer ("dualview/previewExpressions") is currently completely undocumented and the only way to determine its function is to read the code (which is sparsely commented). I've read though the code and determined this property's function; the final form of the documentation for it might be:

 * - "dualview/previewExpressions"
 *       List of recently-used expressions used to determine how features are
 *       displayed in the list at the left side of the Attribute Table window
 *       when in "form" view.  These expressions are displayed at the end of 
 *       the Expression drop-down menu.  [QgsDualView]

Thoughts?

Copy link
Contributor Author

@Alex-Kent Alex-Kent Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thought, I'd add something like the following to the comment in each of the above header files:

 * If using custom properties to store settings for a plugin it is recommended
 * that one use a unique prefix for its settings, e.g. "MyPlugin/property_name",
 * as doing so will lessen the chance of conflicts with property names used by
 * other code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue as I see it is that custom properties are stored within e.g. a QgsMapLayer but are managed from outside that class. In some cases it's pretty clear where and how the property is being used (as with userNotes in QgsMapLayer). In other cases though (as with dualview/previewExpressions) it's not clear where and how the property is being used.

For most class variables it's clear which variables map to what XML as well as which code reads/writes the variables; this is true in the class header file, the class source, and the project file's XML. This isn't the case with custom properties which can be arbitrarily set from anywhere within the code (including from seemingly unrelated classes); this makes determining their function and the code that uses them non-trivial.

Hopefully I'm not coming across as sophomoric. I think having these documented would be a good thing and I'm trying to be clear why I think so.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with some of what you said but I think there are good reasons to have these documented.

it goes out of sync very quickly

As these properties are written to every project file one would expect the existing ones to remain valid in perpetuity. New properties might be added in future which are not included in this list and I agree there's no good way to handle that case.

I'm referring to the code locations. Code is shuffled frequently in QGIS, and no-one will be aware that moving this code results in documentation from a completely different location becoming out of date.

a lot of these methods are private methods

These may be written/read by private methods but the resulting project files are text files that a user might want to parse or edit. (I've personally done both.)

Again, I'm referring to the code locations. It's rather frustrating to see things like this in documentation, where you read about something interesting but then it's locked away in a private method that you can't even access 🙃

Ideally all properties would have a description of their function in the header file
agreed

Listing the methods where the values are set is a temporary measure and meant primarily as an aid to writing a proper description [currently the only way to determine which properties exist and their function is by manually reading through the source]; knowing what code writes each property makes it easier to determine precisely what values are being stored. Once descriptions have been written for each property then the lists in this PR that map property names to the methods that set them could be removed.

I'd still argue that it's just cluttering the docs with fragile, not very helpful information. If the plan is to add full descriptions for them later, then let's just leave out the extra until you're ready to do that. Chuck the locations into a gist or something for now, but don't add them to the stable API docs.

it's seen as a internal implementation detail only, not something which the user of the API needs to be aware of

I disagree; custom properties are written to the project files and having their meanings documented is a good thing.

Again, I'm (mostly) referring to the code locations here. BUT -- custom properties aren't considered part of stable API, and may be freely changed and reworked in future. Eg at some stage someone might write a QgsProjectFileTransform to rename or move some custom property, and at that stage any code which relies on the hardcoded property names will break. It's a relatively low risk, but it IS possible and allowed.

How about adding a "\warning These properties are not considered stable API, and may change in a future QGIS release without warning" to the docs for these methods?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thought, I'd add something like the following to the comment in each of the above header files:

  • If using custom properties to store settings for a plugin it is recommended
  • that one use a unique prefix for its settings, e.g. "MyPlugin/property_name",
  • as doing so will lessen the chance of conflicts with property names used by
  • other code.

Sounds like a good idea! (You can use a \note xxx annotation for things like this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants