Skip to content

Group all memories and other sync improvements #1021

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.

Sign up for GitHub

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

Merged
beastoin merged 4 commits into from
Oct 10, 2024
Merged

Group all memories and other sync improvements #1021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe2195c
keep all mems as grouped and wal handling imrpovements
mdmohsin7 Oct 10, 2024
795eaf2
view synced memory details
mdmohsin7 Oct 10, 2024
2bb6737
directly use syncedMemoriesPointers
mdmohsin7 Oct 10, 2024
8a0b5b1
filtr memories on front
mdmohsin7 Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions app/lib/pages/memories/page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,13 @@ class _MemoriesPageState extends State<MemoriesPage> with AutomaticKeepAliveClie
children: [
if (index == 0) const SizedBox(height: 16),
DateListItem(date: date, isFirst: index == 0),
...memoriesForDate.map(
(memory) => MemoryListItem(
memory: memory,
memoryIdx: memoryProvider.groupedMemories[date]!.indexOf(memory),
date: date,
),
),
...memoriesForDate.where((mem) => memoryProvider.showDiscardedMemories || !mem.discarded).map(
(memory) => MemoryListItem(
memory: memory,
memoryIdx: memoryProvider.groupedMemories[date]!.indexOf(memory),
date: date,
),
),
Comment on lines +142 to +148
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 10, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Issue: showDiscardedMemories changes do not trigger UI updates.

The showDiscardedMemories property changes do not call notifyListeners(), preventing the UI from rebuilding as expected.

  • Ensure that the setter for showDiscardedMemories includes a call to notifyListeners(). For example: 
    set showDiscardedMemories(bool value) {
  _showDiscardedMemories = value;
  notifyListeners();
}
🔗 Analysis chain

LGTM! Consider adding a comment and verifying UI updates.

The implementation of the filtering mechanism for discarded memories looks good and aligns with the PR objectives. Great job on improving the memory management!

A few suggestions to enhance the code:

  1. Consider adding a comment explaining the filtering logic for better code readability. For example:

    // Show all memories if showDiscardedMemories is true, otherwise show only non-discarded memories

  2. Ensure that changing the showDiscardedMemories property triggers a rebuild of the widget. You may want to verify this behavior:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if showDiscardedMemories is properly observed for UI updates

# Test: Search for notifyListeners() call when showDiscardedMemories is changed
rg --type dart 'set showDiscardedMemories.*notifyListeners' app/lib/providers/

# Test: Check if MemoriesPage is listening to changes in MemoryProvider
rg --type dart 'Consumer<MemoryProvider>' app/lib/pages/memories/

Length of output: 555

],
);
}
Expand Down
6 changes: 3 additions & 3 deletions app/lib/pages/memories/sync_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class _SyncPageState extends State<SyncPage> {
textAlign: TextAlign.center,
),
)
: memoryProvider.syncCompleted && memoryProvider.syncResult != null
: memoryProvider.syncCompleted && memoryProvider.syncedMemoriesPointers != null
? Column(
children: [
const Text(
Expand All @@ -125,8 +125,8 @@ class _SyncPageState extends State<SyncPage> {
const SizedBox(
height: 18,
),
(memoryProvider.syncResult!['new_memories'].isNotEmpty ||
memoryProvider.syncResult!['updated_memories'].isNotEmpty)
(memoryProvider.syncedMemoriesPointers!['new_memories']!.isNotEmpty ||
memoryProvider.syncedMemoriesPointers!['updated_memories']!.isNotEmpty)
? Container(
padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 0),
decoration: BoxDecoration(
Expand Down
3 changes: 1 addition & 2 deletions app/lib/pages/memories/synced_memories_page.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ import 'package:provider/provider.dart';
import 'widgets/synced_memory_list_item.dart';

class SyncedMemoriesPage extends StatelessWidget {
final Map<String, dynamic>? res;
const SyncedMemoriesPage({super.key, this.res});
const SyncedMemoriesPage({super.key});

@override
Widget build(BuildContext context) {
Expand Down
11 changes: 10 additions & 1 deletion app/lib/pages/memories/widgets/synced_memory_list_item.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import 'package:flutter/material.dart';
import 'package:friend_private/backend/http/api/memories.dart';
import 'package:friend_private/backend/schema/memory.dart';
import 'package:friend_private/pages/memory_detail/memory_detail_provider.dart';
import 'package:friend_private/pages/memory_detail/page.dart';
import 'package:friend_private/providers/memory_provider.dart';
import 'package:friend_private/utils/other/temp.dart';
import 'package:provider/provider.dart';
Expand Down Expand Up @@ -54,7 +56,14 @@ class _SyncedMemoryListItemState extends State<SyncedMemoryListItem> {
}

return GestureDetector(
onTap: () async {},
onTap: () async {
context.read<MemoryDetailProvider>().updateMemory(widget.memoryIdx, widget.date);
Provider.of<MemoryProvider>(context, listen: false).onMemoryTap(widget.memoryIdx);
routeToPage(
context,
MemoryDetailPage(memory: widget.memory, isFromOnboarding: false),
);
},
Comment on lines +59 to +66
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 10, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use consistent methods to access providers

In line 60, you're using context.read<MemoryDetailProvider>(), while in line 61, you're using Provider.of<MemoryProvider>(context, listen: false). For consistency and readability, consider using the same method to access providers throughout your code, such as context.read<T>() or Provider.of<T>(context, listen: false).

child: Padding(
padding: const EdgeInsets.only(top: 12, left: 16, right: 16),
child: Container(
Expand Down
72 changes: 44 additions & 28 deletions app/lib/providers/memory_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal

bool isLoadingMemories = false;
bool hasNonDiscardedMemories = true;
bool showDiscardedMemories = false;

String previousQuery = '';

Expand All @@ -36,8 +37,6 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal

bool isSyncing = false;
bool syncCompleted = false;
Map<String, dynamic>? syncResult;
Map<String, List<ServerMemory?>>? syncedMemories;
Map<String, List<Record>>? syncedMemoriesPointers;

MemoryProvider() {
Expand Down Expand Up @@ -77,7 +76,8 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
void toggleDiscardMemories() {
MixpanelManager().showDiscardedMemoriesToggled(!SharedPreferencesUtil().showDiscardedMemories);
SharedPreferencesUtil().showDiscardedMemories = !SharedPreferencesUtil().showDiscardedMemories;
filterGroupedMemories('');
showDiscardedMemories = SharedPreferencesUtil().showDiscardedMemories;
// filterGroupedMemories('');
notifyListeners();
}

Expand All @@ -92,7 +92,6 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
processingMemories = memories.where((m) => m.status == MemoryStatus.processing).toList();

memories = memories.where((m) => m.status == MemoryStatus.completed).toList();

if (memories.isEmpty) {
memories = SharedPreferencesUtil().cachedMemories;
} else {
Expand All @@ -105,7 +104,7 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
void _groupMemoriesByDateWithoutNotify() {
groupedMemories = {};
for (var memory in memories) {
if (SharedPreferencesUtil().showDiscardedMemories && memory.discarded && !memory.isNew) continue;
// if (SharedPreferencesUtil().showDiscardedMemories && memory.discarded && !memory.isNew) continue;
var date = DateTime(memory.createdAt.year, memory.createdAt.month, memory.createdAt.day);
if (!groupedMemories.containsKey(date)) {
groupedMemories[date] = [];
Expand Down Expand Up @@ -144,7 +143,8 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
}

void filterGroupedMemories(String query) {
_filterGroupedMemoriesWithoutNotify(query);
// _filterGroupedMemoriesWithoutNotify(query);
_groupMemoriesByDateWithoutNotify();
notifyListeners();
}

Expand Down Expand Up @@ -345,13 +345,8 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
setIsSyncing(true);
var res = await _walService.syncAll(progress: this);
if (res != null) {
syncResult = {"new_memories": res.newMemoryIds, 'updated_memories': res.updatedMemoryIds};
}
if (syncResult != null) {
if (syncResult!['new_memories'] != [] || syncResult!['updated_memories']) {
syncedMemories = {};
await getSyncedMemoriesData();
addSyncedMemoriesToGroupedMemories();
if (res.newMemoryIds.isNotEmpty || res.updatedMemoryIds.isNotEmpty) {
await getSyncedMemoriesData(res);
}
}
syncCompleted = true;
Expand All @@ -360,39 +355,51 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
return;
}

Future getSyncedMemoriesData() async {
List<dynamic> newMemories = syncResult!['new_memories'] ?? [];
List<dynamic> updatedMemories = syncResult!['updated_memories'] ?? [];
Future getSyncedMemoriesData(SyncLocalFilesResponse syncResult) async {
List<dynamic> newMemories = syncResult.newMemoryIds;
List<dynamic> updatedMemories = syncResult.updatedMemoryIds;

List<Future<ServerMemory?>> newMemoriesFutures = newMemories.map((item) => getMemoryDetails(item)).toList();

List<Future<ServerMemory?>> updatedMemoriesFutures = updatedMemories.map((item) => getMemoryDetails(item)).toList();
var syncedMemories = {'new_memories': [], 'updated_memories': []};
try {
final newMemoriesResponses = await Future.wait(newMemoriesFutures);
syncedMemories!['new_memories'] = newMemoriesResponses;
syncedMemories['new_memories'] = newMemoriesResponses;

final updatedMemoriesResponses = await Future.wait(updatedMemoriesFutures);
syncedMemories!['updated_memories'] = updatedMemoriesResponses;
syncedMemories['updated_memories'] = updatedMemoriesResponses;
addSyncedMemoriesToGroupedMemories(syncedMemories);
} catch (e) {
print('Error during API calls: $e');
}
}

void addSyncedMemoriesToGroupedMemories() {
void addSyncedMemoriesToGroupedMemories(Map syncedMemories) {
syncedMemoriesPointers = {'new_memories': [], 'updated_memories': []};
if (syncedMemories == null) return;
if (syncedMemories!['new_memories'] != null) {
for (var memory in syncedMemories!['new_memories']!) {
if (memory != null) {
if (syncedMemories['new_memories'] != []) {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 10, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix list emptiness checks by using .isNotEmpty

In Dart, comparing a list directly to an empty list using != [] does not yield the expected result because it compares references, not content. Instead, use the .isNotEmpty property to check if a list is not empty.

Apply this diff to correct the checks:

- if (syncedMemories['new_memories'] != []) {
+ if (syncedMemories['new_memories'].isNotEmpty) {

- if (syncedMemories['updated_memories'] != []) {
+ if (syncedMemories['updated_memories'].isNotEmpty) {

Also applies to: 387-387

for (var memory in syncedMemories['new_memories']!) {
if (memory != null && memory.status == MemoryStatus.completed) {
addMemory(memory);
syncedMemoriesPointers!['new_memories']!.add(getMemoryDateAndIndex(memory));
}
}
}
if (syncedMemories!['updated_memories'] != null) {
for (var memory in syncedMemories!['updated_memories']!) {
if (syncedMemories['updated_memories'] != []) {
for (var memory in syncedMemories['updated_memories']!) {
if (memory != null && memory.status == MemoryStatus.completed) {
upsertMemory(memory);
syncedMemoriesPointers!['updated_memories']!.add(getMemoryDateAndIndex(memory));
}
}
}
for (var memory in syncedMemories['new_memories']!) {
if (memory != null && memory.status == MemoryStatus.completed) {
syncedMemoriesPointers!['new_memories']!.add(getMemoryDateAndIndex(memory));
}
}
if (syncedMemories['updated_memories'] != []) {
for (var memory in syncedMemories['updated_memories']!) {
if (memory != null && memory.status == MemoryStatus.completed) {
updateMemoryInSortedList(memory);
syncedMemoriesPointers!['updated_memories']!.add(getMemoryDateAndIndex(memory));
}
}
Expand All @@ -410,6 +417,16 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
syncedMemoriesPointers!['updated_memories']![idx] = getMemoryDateAndIndex(memory);
}
}
// if new_memories also include the same memory, update it coz we currently have duplicates
if (syncedMemoriesPointers!['new_memories'] != null) {
var idx = syncedMemoriesPointers!['new_memories']!.indexWhere((element) {
dynamic e = element;
return e.$3.id == memory.id;
});
Comment on lines +424 to +425
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Oct 10, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use named fields in records for better readability

Accessing record fields using positional notation like e.\$3 can reduce code clarity and increase the chance of errors. Consider using named fields in your records to enhance readability and maintainability.

Modify the return type of getMemoryDateAndIndex to use named fields:

({DateTime date, int index, ServerMemory memory}) getMemoryDateAndIndex(ServerMemory memory) {
  // implementation
}

Then access the fields using:

return e.memory.id == memory.id;

if (idx != -1) {
syncedMemoriesPointers!['new_memories']![idx] = getMemoryDateAndIndex(memory);
}
}
}

(DateTime, int, ServerMemory) getMemoryDateAndIndex(ServerMemory memory) {
Expand All @@ -427,7 +444,6 @@ class MemoryProvider extends ChangeNotifier implements IWalServiceListener, IWal
}

void clearSyncResult() {
syncResult = null;
syncCompleted = false;
notifyListeners();
}
Expand Down