Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8b549396 authored by Ned Burns's avatar Ned Burns
Browse files

Fix overlogging of filter and section changes 

The addition of a second round of filtering introduced a bug that would
cause the system to log false filter and section changes for all notifs
on every run of the pipeline.

Previously, we had logic like the following:
(assume that, in the previous run, entryA was filtered out by filter1
during the pre-finalized stage)

1. preGroupFilters run, nothing returns true. Set
entryA.mExcludingFilter to null. It changed! Log it.
2. preFinalizeFilters run, filter1 returns true. Set
entryA.mExcludingFilter to it. It changed! Log it.

NotifSections had a similar bug that would null out their assigned
section every time they were filtered out.

To solve this, we introduce an mPreviousExcludingFilter field on
NotificationEntry that will allow us to determine whether the filter
actually changed as compared to the previous run. We also move that
logging logic to the end of the pipeline.

To solve the sectioning issue, we just don't null out sections when the
entry is filtered out. Long-term, we should move to a system similar to
the one above, but that's more complicated.

Test: atest
Bug: 112656837
Change-Id: Ib78dbbb0346947210cc1b6b49547595895e9fce5
parent f1d96d48

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/NotificationEntry.java

+6 −0
Original line number Diff line number Diff line
@@ -108,6 +108,12 @@ public final class NotificationEntry extends ListEntry { 
    /** If this notification was filtered out, then the filter that did the filtering. */

    @Nullable NotifFilter mExcludingFilter;



    /**

     * The NotifFilter, if any, that was active on this notification during the previous run of

     * the list builder.

     */

    @Nullable NotifFilter mPreviousExcludingFilter;



    /** If this was a group child that was promoted to the top level, then who did the promoting. */

    @Nullable NotifPromoter mNotifPromoter;

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/ShadeListBuilder.java

+24 −19
Original line number Diff line number Diff line
@@ -316,6 +316,7 @@ public class ShadeListBuilder implements Dumpable { 


        // Step 7: Lock in our group structure and log anything that's changed since the last run

        mPipelineState.incrementTo(STATE_FINALIZING);

        logFilterChanges();

        logParentingChanges();

        freeEmptyGroups();
@@ -363,6 +364,9 @@ public class ShadeListBuilder implements Dumpable { 
            entry.setPreviousParent(entry.getParent());

            entry.setParent(null);



            entry.mPreviousExcludingFilter = entry.mExcludingFilter;

            entry.mExcludingFilter = null;



            if (entry.mFirstAddedIteration == -1) {

                entry.mFirstAddedIteration = mIterationCount;

            }
@@ -371,8 +375,10 @@ public class ShadeListBuilder implements Dumpable { 
        mNotifList.clear();

    }



    private void filterNotifs(Collection<? extends ListEntry> entries,

            List<ListEntry> out, List<NotifFilter> filters) {

    private void filterNotifs(

            Collection<? extends ListEntry> entries,

            List<ListEntry> out,

            List<NotifFilter> filters) {

        final long now = mSystemClock.uptimeMillis();

        for (ListEntry entry : entries)  {

            if (entry instanceof GroupEntry) {
@@ -585,8 +591,9 @@ public class ShadeListBuilder implements Dumpable { 
     * filtered out during any of the filtering steps.

     */

    private void annulAddition(ListEntry entry) {

        entry.setSection(-1);

        entry.mNotifSection = null;

        // TODO: We should null out the entry's section and promoter here. However, if we do that,

        //  future runs will think that the section changed. We need a mPreviousNotifSection,

        //  similar to what we do for parents.

        entry.setParent(null);

        if (entry.mFirstAddedIteration == mIterationCount) {

            entry.mFirstAddedIteration = -1;
@@ -615,6 +622,17 @@ public class ShadeListBuilder implements Dumpable { 
        mGroups.values().removeIf(ge -> ge.getSummary() == null && ge.getChildren().isEmpty());

    }



    private void logFilterChanges() {

        for (NotificationEntry entry : mAllEntries) {

            if (entry.mExcludingFilter != entry.mPreviousExcludingFilter) {

                mLogger.logFilterChanged(

                        entry.getKey(),

                        entry.mPreviousExcludingFilter,

                        entry.mExcludingFilter);

            }

        }

    }



    private void logParentingChanges() {

        for (NotificationEntry entry : mAllEntries) {

            if (entry.getParent() != entry.getPreviousParent()) {
@@ -680,21 +698,8 @@ public class ShadeListBuilder implements Dumpable { 
    };



    private boolean applyFilters(NotificationEntry entry, long now, List<NotifFilter> filters) {

        NotifFilter filter = findRejectingFilter(entry, now, filters);



        if (filter != entry.mExcludingFilter) {

            mLogger.logFilterChanged(

                    entry.getKey(),

                    entry.mExcludingFilter != null ? entry.mExcludingFilter.getName() : null,

                    filter != null ? filter.getName() : null);



            // Note that groups and summaries can also be filtered out later if they're part of a

            // malformed group. We currently don't have a great way to track that beyond parenting

            // change logs. Consider adding something similar to mExcludingFilter for them.

            entry.mExcludingFilter = filter;

        }



        return filter != null;

        entry.mExcludingFilter = findRejectingFilter(entry, now, filters);

        return entry.mExcludingFilter != null;

    }



    @Nullable private static NotifFilter findRejectingFilter(NotificationEntry entry, long now,

packages/SystemUI/src/com/android/systemui/statusbar/notification/collection/listbuilder/ShadeListBuilderLogger.kt

+5 −4
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ import com.android.systemui.log.LogLevel.WARNING 
import com.android.systemui.log.dagger.NotificationLog

import com.android.systemui.statusbar.notification.collection.GroupEntry

import com.android.systemui.statusbar.notification.collection.ListEntry

import com.android.systemui.statusbar.notification.collection.listbuilder.pluggable.NotifFilter

import javax.inject.Inject



class ShadeListBuilderLogger @Inject constructor(
@@ -126,13 +127,13 @@ class ShadeListBuilderLogger @Inject constructor( 


    fun logFilterChanged(

        key: String,

        prevFilter: String?,

        newFilter: String?

        prevFilter: NotifFilter?,

        newFilter: NotifFilter?

    ) {

        buffer.log(TAG, INFO, {

            str1 = key

            str2 = prevFilter

            str3 = newFilter

            str2 = prevFilter?.name

            str3 = newFilter?.name

        }, {

            "Filter changed for $str1: $str2 -> $str3"

        })

packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/collection/NotificationEntryBuilder.java

+1 −1
Original line number Diff line number Diff line
@@ -48,7 +48,7 @@ public class NotificationEntryBuilder { 


    /* ListEntry properties */

    private GroupEntry mParent;

    private int mSection;

    private int mSection = -1;



    public NotificationEntry build() {

        StatusBarNotification sbn = mSbn != null ? mSbn : mSbnBuilder.build();