Diff for 'Sticky at top of lists' does not work

Indeed, bug confirmed.
Checked also revision data and it is saved correct for sticky.

I will check this.

Debugging I see that the field sticky of the node doesn't have a view display configuration, so when trying to load the plugin for it, the code first checks if the field is added to the entity view, if not just skip it when diffing.

Hm, How many fields are filtered out by this approach?
It's possible i'm mixing things...
We also are checking field access to make sure there is no leak.

Just for my understanding: What would happen if we skip this View Mode check completely?
Users have the Visual diff where they can see output differences.
Say, this check might make sense for consideration of the default plugin.
But if a user configures a field plugin, it should be always considered, or not?

In DiffBuilderManager::getSelectedPluginForFieldDefinition this code filters the fields that are not in the view of the entity.
The elseif case checks that when is a BaseFieldDefinition it should get its view.

Removing this else if case fields like:
Revision timestamp
Revision user ID
Paragraphs > Nested Content > Language code
Paragraphs > Nested Content > Authored by
Paragraphs > Nested Content > Published
Paragraphs > Nested Content > Authored on
Paragraphs > Nested Content > Revision user ID
Paragraphs > Nested Content > Default translation
and others are not filtered.

if ($target_bundle = $field_definition->getTargetBundle()) {
      $view_mode = $this->config->get('content_type_settings.' . $target_bundle . '.view_mode');
      // If no view mode is selected use the default view mode.
      if ($view_mode == NULL) {
        $view_mode = 'default';
      }
      if ($display = $storage->load($field_definition->getTargetEntityTypeId() . '.' . $target_bundle . '.' . $view_mode)) {
        $visible = (bool) $display->getComponent($field_definition->getName());
      }
    }
    elseif ($field_definition instanceof BaseFieldDefinition) {
      if (!$display = $field_definition->getDisplayOptions('view')) {
        $visible = FALSE;
      }
    }

If an user configures a field plugin it should be considered as long as the field is part of the entity view IMHO.

Agree about functionality. Your argument works for fields like this "sticky". It doesn't work though for "changed" field that changes always and should not be displayed...
The Drupal API doesn't allow us to make a difference between fields with user awareness or not.

Anyway, let's ignore the default setting topic for a second.
There is the admin diff field settings. If i configure a diff plugin, it should be output in the diff. No matter if this is displayed or not. The unified fields are more a technical view for field change. If you want to see visual changes, you use the visual field diff. This considers view mode display. If it is not enough to see, i switch to the field diff...

How about if we consider using the default form mode for default selection? If a field appears in the form, we propose a diff plugin as default.

I want to iron this out for the release.
Diff doesn't do its job if a field change like this can't be visualised.

Like discussed, we want to test...
- If is better to look at form mode
- If unwanted things appear if we drop the view mode check completely

We might need hardcoded exclusion for the revision uid and changed field as they don't make sense to diff.

When changing to check the view instead of the form, the sticky is displayed and no other useless fields are filling the diff with useless info.

I also tested with Paragraphs and realized that the fields we had problems before are also problematic due they have are form configurable, if we remove that the diff will work flawlessly IMHO:

I will support this concept with the idea of "What you edit, what you diff".

When removing the view check, some fields that this timestamp are shown. Not really sure about this approach.

Indeed, revision timestamp and revision author are special fields. They exist per revision, but are technical no "content" that is revised.
They need to be manually excluded/filtered from comparision.
In fact, if you check node_revision table, it's all of them: nid, vid, revision_timestamp, revision_uid, revision_log.

Discussed with Berdir and we both agree:
- If some setting is explicitly selected and stored, it needs to be considered. no fallback.
- But if there is no setting, we still should figure out a great default.

The content type field display visibility was always only intended to be considered for the fallback / default selection.
We believe looking up for the visibility in the default form display is an even better default setting proposal.

Changed to use the entity form display, and when settings are set it just skip all the form display verification. Having problems with Base Fields due when loading the displayOptions('form') it is always present even the field is hidden. Not sure how to change that.

Fixing some tests, discussed with @Berdir, we gonna remove that diff view mode and then fix remaining tests.

Patch was missing, now all test should pass.

+++ b/src/Tests/DiffRevisionTest.php
@@ -231,7 +231,8 @@ class DiffRevisionTest extends DiffTestBase {
+    $this->assertText('Changes on: Title, Revision log message, Body');

Changes on "Revision log message"?

That field, and the revision author, need to be excluded hardcoded from the diff fields. It's metadata about a specific revision, nothing to compare.

Will check rest in more detail later.

Added some small changes to exclude revision log and revision author from the diff / summary, by now hard coding the field names, added a @todo for following the related issue.

Yeah, lots of things read nice. Still more to deal with:

+++ b/src/DiffBuilderManager.php
@@ -128,31 +133,38 @@ class DiffBuilderManager extends DefaultPluginManager {
-      if (!$display = $field_definition->getDisplayOptions('view')) {
...
+      if ($entity) {
...
+        $view_mode = $this->config->get('content_type_settings.' . $entity->bundle() . '.view_mode');
...
+        if ($display = $storage->load($entity->getEntityTypeId() . '.' . $entity->bundle() . '.' . $view_mode)) {

The FieldDefinitionInterface would have a getTargetEntityTypeId() and getTargetBundle().
So it seems to me you don't need to change the interface as we don't need $entity at all?

+++ b/src/DiffBuilderManager.php
@@ -128,31 +133,38 @@ class DiffBuilderManager extends DefaultPluginManager {
+        $view_mode = $this->config->get('content_type_settings.' . $entity->bundle() . '.view_mode');
+        // If no view mode is selected use the default view mode.
+        if ($view_mode == NULL) {
+          $view_mode = 'default';
+        }

Hm,i guess i got it wrong. I thought you want to remove that setting completely in #20? Is this part of some other issue?

About #24.1, of the previous comment I think is not possible because the base fields don't have a target Bundle. That's why when discussing with @Berdir we realized that might be better to get the entity for checking the display.
#24.2 is a setting that will be removed in another issue.

+++ b/src/DiffBuilderManager.php
@@ -102,13 +104,16 @@ class DiffBuilderManager extends DefaultPluginManager {
+  public function createInstanceForFieldDefinition(FieldDefinitionInterface $field_definition, ContentEntityInterface $entity) {

@@ -128,31 +133,38 @@ class DiffBuilderManager extends DefaultPluginManager {
-      if (!$display = $field_definition->getDisplayOptions('view')) {

I still feel this is a strange API definition: Getting a plugin for field definition doesn't need an entity instance.
Instead if the field definition isn't enough.. shouldn't we pass in the entity type ID and the bundle..?
What do you think?

This should be better. Discussed with @Berdir, it might be OK to use the bundle in there, but just to be sure was set as optional.

Yes, berdir said to use the *bundle*.

You still use the entity. only pass in a bundle string.

:/ sorry, my bad. Now should be better.

Wonderful, committed. :-)