Make forcedVisible logic act on threads, not annotations

Change `forcedVisibleAnnotations` => `forcedVisibleThreads` and
use thread ids instead of annotation $tags.

This allows the forcing-visible of threads that lack an
annotation and is more appropriate overall: threads are
forced-visible, not annotations.

Author: lyzadanger

src/sidebar/components/FilterStatus.js

@@ -279,7 +279,7 @@ export default function FilterStatus() {
 
   const store = useStoreProxy();
   const focusState = store.focusState();
-  const forcedVisibleCount = store.forcedVisibleAnnotations().length;
+  const forcedVisibleCount = store.forcedVisibleThreads().length;
   const filterQuery = store.filterQuery();
   const selectedCount = store.selectedAnnotations().length;
 

src/sidebar/components/NotebookResultCount.js

@@ -18,7 +18,7 @@ import Spinner from './Spinner';
 function NotebookResultCount() {
   const store = useStoreProxy();
 
-  const forcedVisibleCount = store.forcedVisibleAnnotations().length;
+  const forcedVisibleCount = store.forcedVisibleThreads().length;
   const hasAppliedFilter = store.hasAppliedFilter();
   const resultCount = store.annotationResultCount();
   const isLoading = store.isLoading();

src/sidebar/components/test/FilterStatus-test.js

@@ -44,7 +44,7 @@ describe('FilterStatus', () => {
       filterQuery: sinon.stub().returns(null),
       filterState: sinon.stub().returns(getFilterState()),
       focusState: sinon.stub().returns(getFocusState()),
-      forcedVisibleAnnotations: sinon.stub().returns([]),
+      forcedVisibleThreads: sinon.stub().returns([]),
       selectedAnnotations: sinon.stub().returns([]),
       toggleFocusMode: sinon.stub(),
     };
@@ -116,7 +116,7 @@ describe('FilterStatus', () => {
   context('(State 3): filtered by query with force-expanded threads', () => {
     beforeEach(() => {
       fakeStore.filterQuery.returns('foobar');
-      fakeStore.forcedVisibleAnnotations.returns([1, 2, 3]);
+      fakeStore.forcedVisibleThreads.returns([1, 2, 3]);
       fakeThreadUtil.countVisible.returns(5);
     });
 
@@ -284,7 +284,7 @@ describe('FilterStatus', () => {
           displayName: 'Ebenezer Studentolog',
         });
         fakeStore.filterQuery.returns('biscuits');
-        fakeStore.forcedVisibleAnnotations.returns([1, 2]);
+        fakeStore.forcedVisibleThreads.returns([1, 2]);
         fakeThreadUtil.countVisible.returns(3);
       });
 
@@ -331,7 +331,7 @@ describe('FilterStatus', () => {
         configured: true,
         displayName: 'Ebenezer Studentolog',
       });
-      fakeStore.forcedVisibleAnnotations.returns([1, 2, 3]);
+      fakeStore.forcedVisibleThreads.returns([1, 2, 3]);
       fakeThreadUtil.countVisible.returns(7);
     });
 
@@ -343,7 +343,7 @@ describe('FilterStatus', () => {
     });
 
     it('should handle cases when there are no focused-user annotations', () => {
-      fakeStore.forcedVisibleAnnotations.returns([1, 2, 3, 4, 5, 6, 7]);
+      fakeStore.forcedVisibleThreads.returns([1, 2, 3, 4, 5, 6, 7]);
       assertFilterText(
         createComponent(),
         'No annotations by Ebenezer Studentolog (and 7 more)'

src/sidebar/components/test/NotebookResultCount-test.js

@@ -22,7 +22,7 @@ describe('NotebookResultCount', () => {
     fakeAnnotationResultCount = sinon.stub().returns(0);
 
     fakeStore = {
-      forcedVisibleAnnotations: sinon.stub().returns([]),
+      forcedVisibleThreads: sinon.stub().returns([]),
       hasAppliedFilter: sinon.stub().returns(false),
       isLoading: fakeIsLoading,
       annotationResultCount: fakeAnnotationResultCount,
@@ -111,7 +111,7 @@ describe('NotebookResultCount', () => {
     ].forEach(test => {
       it('should render a count of results', () => {
         fakeStore.hasAppliedFilter.returns(true);
-        fakeStore.forcedVisibleAnnotations.returns(test.forcedVisible);
+        fakeStore.forcedVisibleThreads.returns(test.forcedVisible);
         fakeUseRootThread.returns(test.thread);
         fakeCountVisible.returns(test.visibleCount);
 

src/sidebar/helpers/build-thread.js

@@ -296,9 +296,7 @@ export default function buildThread(annotations, options) {
     thread.children = thread.children.filter(child => {
       const isSelected = options.selected.includes(child.id);
       const isForcedVisible =
-        hasForcedVisible &&
-        child.annotation &&
-        options.forcedVisible.includes(child.annotation.$tag);
+        hasForcedVisible && options.forcedVisible.includes(child.id);
       return isSelected || isForcedVisible;
     });
   }
@@ -312,19 +310,16 @@ export default function buildThread(annotations, options) {
   thread = mapThread(thread, thread => {
     let threadIsVisible = thread.visible;
 
-    if (!thread.annotation) {
-      threadIsVisible = false; // Nothing to show
-    } else if (options.filterFn) {
-      if (
-        hasForcedVisible &&
-        options.forcedVisible.includes(thread.annotation.$tag)
-      ) {
-        // This annotation may or may not match the filter, but we should
+    if (options.filterFn) {
+      if (hasForcedVisible && options.forcedVisible.includes(thread.id)) {
+        // This thread may or may not match the filter, but we should
         // make sure it is visible because it has been forced visible by user
         threadIsVisible = true;
-      } else {
-        // Otherwise, visibility depends on whether it matches the filter
+      } else if (thread.annotation) {
+        // Otherwise, visibility depends on whether its annotation matches the filter
         threadIsVisible = !!options.filterFn(thread.annotation);
+      } else {
+        threadIsVisible = false;
       }
     }
     return { ...thread, visible: threadIsVisible };

src/sidebar/helpers/thread.js

@@ -13,8 +13,7 @@ import { notNull } from '../util/typing';
  * @return {number}
  */
 function countByVisibility(thread, visibility) {
-  const matchesVisibility =
-    !!thread.annotation && thread.visible === visibility;
+  const matchesVisibility = thread.visible === visibility;
   return thread.children.reduce(
     (count, reply) => count + countByVisibility(reply, visibility),
     matchesVisibility ? 1 : 0

src/sidebar/services/test/threads-test.js

@@ -2,32 +2,30 @@ import threadsService from '../threads';
 
 const NESTED_THREADS = {
   id: 'top',
-  annotation: { $tag: 'top-tag' },
+  annotation: {},
   children: [
     {
       id: '1',
-      annotation: { $tag: '1-tag' },
+      annotation: {},
       children: [
         {
           id: '1a',
-          annotation: { $tag: '1a-tag' },
-          children: [
-            { annotation: { $tag: '1ai-tag' }, id: '1ai', children: [] },
-          ],
+          annotation: {},
+          children: [{ annotation: {}, id: '1ai', children: [] }],
         },
         {
           id: '1b',
-          annotation: { $tag: '1b-tag' },
+          annotation: {},
           children: [],
           visible: true,
         },
         {
           id: '1c',
-          annotation: { $tag: '1c-tag' },
+          annotation: {},
           children: [
             {
               id: '1ci',
-              annotation: { $tag: '1ci-tag' },
+              annotation: {},
               children: [],
               visible: false,
             },
@@ -37,32 +35,32 @@ const NESTED_THREADS = {
     },
     {
       id: '2',
-      annotation: { $tag: '2-tag' },
+      annotation: {},
       children: [
-        { id: '2a', annotation: { $tag: '2a-tag' }, children: [] },
+        { id: '2a', annotation: {}, children: [] },
         {
           id: '2b',
           children: [
             {
               id: '2bi',
-              annotation: { $tag: '2bi-tag' },
+              annotation: {},
               children: [],
               visible: true,
             },
-            { id: '2bii', annotation: { $tag: '2bii-tag' }, children: [] },
+            { id: '2bii', annotation: {}, children: [] },
           ],
         },
       ],
     },
     {
       id: '3',
-      annotation: { $tag: '3-tag' },
+      annotation: {},
       children: [],
     },
   ],
 };
 
-describe('threadsService', function () {
+describe('threadsService', () => {
   let fakeStore;
   let service;
 
@@ -77,17 +75,17 @@ describe('threadsService', function () {
     let nonVisibleThreadIds;
     beforeEach(() => {
       nonVisibleThreadIds = [
-        'top-tag',
-        '1-tag',
-        '2-tag',
-        '3-tag',
-        '1a-tag',
-        '1c-tag',
-        '2a-tag',
-        //'2b-tag',  Not visible, but also does not have an annotation
-        '1ai-tag',
-        '1ci-tag',
-        '2bii-tag',
+        'top',
+        '1',
+        '2',
+        '3',
+        '1a',
+        '1c',
+        '2a',
+        '2b',
+        '1ai',
+        '1ci',
+        '2bii',
       ];
     });
     it('should set the thread and its children force-visible in the store', () => {
@@ -109,13 +107,7 @@ describe('threadsService', function () {
       for (let i = 0; i < fakeStore.setForcedVisible.callCount; i++) {
         calledWithThreadIds.push(fakeStore.setForcedVisible.getCall(i).args[0]);
       }
-      assert.deepEqual(calledWithThreadIds, [
-        '1ai-tag',
-        '1a-tag',
-        '1ci-tag',
-        '1c-tag',
-        '1-tag',
-      ]);
+      assert.deepEqual(calledWithThreadIds, ['1ai', '1a', '1ci', '1c', '1']);
     });
   });
 });

src/sidebar/services/threads.js

@@ -16,8 +16,8 @@ export default function threadsService(store) {
     thread.children.forEach(child => {
       forceVisible(child);
     });
-    if (!thread.visible && thread.annotation) {
-      store.setForcedVisible(thread.annotation.$tag, true);
+    if (!thread.visible) {
+      store.setForcedVisible(thread.id, true);
     }
   }
 

src/sidebar/store/modules/selection.js

@@ -65,7 +65,7 @@ function init(settings) {
     // until explicitly expanded.
     expanded: initialSelection(settings) || {},
 
-    // Set of annotations that have been "forced" visible by the user
+    // Set of threads that have been "forced" visible by the user
     // (e.g. by clicking on "Show x more" button) even though they may not
     // match the currently-applied filters
     forcedVisible: {},
@@ -261,11 +261,11 @@ function setExpanded(id, expanded) {
 }
 
 /**
- * A user may "force" an annotation to be visible, even if it would be otherwise
+ * A user may "force" an thread to be visible, even if it would be otherwise
  * not be visible because of applied filters. Set the force-visibility for a
- * single annotation, without affecting other forced-visible annotations.
+ * single thread, without affecting other forced-visible threads.
  *
- * @param {string} id
+ * @param {string} id - Thread id
  * @param {boolean} visible - Should this annotation be visible, even if it
  *        conflicts with current filters?
  */
@@ -312,7 +312,7 @@ function expandedMap(state) {
 /**
  * @type {(state: any) => string[]}
  */
-const forcedVisibleAnnotations = createSelector(
+const forcedVisibleThreads = createSelector(
   state => state.forcedVisible,
   forcedVisible => trueKeys(forcedVisible)
 );
@@ -352,7 +352,7 @@ const selectionState = createSelector(
   selection => {
     return {
       expanded: expandedMap(selection),
-      forcedVisible: forcedVisibleAnnotations(selection),
+      forcedVisible: forcedVisibleThreads(selection),
       selected: selectedAnnotations(selection),
       sortKey: sortKey(selection),
       selectedTab: selectedTab(selection),
@@ -404,7 +404,7 @@ export default storeModule({
 
   selectors: {
     expandedMap,
-    forcedVisibleAnnotations,
+    forcedVisibleThreads,
     hasSelectedAnnotations,
     selectedAnnotations,
     selectedTab,

src/sidebar/store/modules/test/selection-test.js

@@ -30,7 +30,7 @@ describe('sidebar/store/modules/selection', () => {
         id1: true,
         id2: false,
       });
-      assert.deepEqual(store.forcedVisibleAnnotations(), ['id1']);
+      assert.deepEqual(store.forcedVisibleThreads(), ['id1']);
     });
   });
 
@@ -154,7 +154,7 @@ describe('sidebar/store/modules/selection', () => {
       });
 
       assert.isEmpty(store.selectedAnnotations());
-      assert.isEmpty(store.forcedVisibleAnnotations());
+      assert.isEmpty(store.forcedVisibleThreads());
     });
   });
 
@@ -166,7 +166,7 @@ describe('sidebar/store/modules/selection', () => {
       store.setFilter('user', { value: 'dingbat', display: 'Ding Bat' });
 
       assert.isEmpty(store.selectedAnnotations());
-      assert.isEmpty(store.forcedVisibleAnnotations());
+      assert.isEmpty(store.forcedVisibleThreads());
     });
   });
 
@@ -178,7 +178,7 @@ describe('sidebar/store/modules/selection', () => {
       store.setFilterQuery('foobar');
 
       assert.isEmpty(store.selectedAnnotations());
-      assert.isEmpty(store.forcedVisibleAnnotations());
+      assert.isEmpty(store.forcedVisibleThreads());
     });
   });
 
@@ -190,7 +190,7 @@ describe('sidebar/store/modules/selection', () => {
       store.toggleFocusMode(true);
 
       assert.isEmpty(store.selectedAnnotations());
-      assert.isEmpty(store.forcedVisibleAnnotations());
+      assert.isEmpty(store.forcedVisibleThreads());
     });
   });
 
@@ -206,7 +206,7 @@ describe('sidebar/store/modules/selection', () => {
         1: true,
         3: true,
       });
-      assert.deepEqual(store.forcedVisibleAnnotations(), ['1']);
+      assert.deepEqual(store.forcedVisibleThreads(), ['1']);
       assert.deepEqual(store.expandedMap(), { 1: true });
     });
   });