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

feat: support editor to show workspace folder overview #4420

Conversation

Jayaprakash-dev
Copy link
Contributor

Description

I've addressed the issue #4282 by implementing the Workspace folder Overview feature, allowing users to create an outline for workspace folder/sub pages structures using the slash commands /overview or /workspace overview. This facilitates easy navigation in nested document structures.

Implemented Changes

  1. Added GetAllLevelOfViews event in the backend to retrieve all levels of child views. Previously, only the GetView event returned first-level children.

  2. Added a struct WorkspaceOverviewListenerIdManager in the Rust backend. This struct stores overview block listener IDs to notify them about updates in child views from all levels. Additionally, a new notification event DidUpdateWorkspaceOverviewChildViews has been added to inform overview listener IDs about changes. This ensures that the struct and overview ID listener are specifically designed to handle updates across all child view levels.

  3. Implemented navigation to pages from the workspace overview block.

The only issue is that we need to assign the view ID to the editor document in AppFlowy-IO/appflowy-editor repo. This enables us to retrieve the view from the backend using the stored view ID, like this:

In Overview block component widget builder

@override
  Widget build(BuildContext context) {
    final viewId = editorState.document.id!;
    return FutureBuilder<Either<ViewPB, FlowyError>>(
      future: ViewBackendService.getAllLevelOfViews(viewId),
      builder: (context, snapshot) { ...... }

In Document class of AppflowyEditor class:

class Document {
  Document({
    required this.root,
    this.id
  });

  /// The root [Node] of the [Document]
  final Node root;

  /// The view Id of the [Document]
  final String? id;
...... }

Testing

I have tested this implementation thoroughly, ensuring it functions as expected and addressing the outlined issue.

Note

Note: There might be some syntax issues in the Rust code as I am a newbie in Rust programming. I welcome suggestions and feedback to improve the code quality.

Screenshots

Screenshot 2024-01-19 at 5 19 34 AM
appflowy_overview_demo.mp4

@LucasXu0
Copy link
Collaborator

@Jayaprakash-dev have you tested the deletion and update cases? Please include integration tests to cover your feature. If you have any questions about the integration feature, feel free to DM me on Discord or comment here.

@Jayaprakash-dev
Copy link
Contributor Author

Jayaprakash-dev commented Jan 19, 2024

@LucasXu0, I've already included an integration test for the update cases in the PR, I will include a test for deletion cases.

@LucasXu0
Copy link
Collaborator

@LucasXu0, I've already included an integration test in the PR.

I meant the insertion and deletion tests when opening the page, and the end of each test should validate the results. Never mind, I will comment directly on the code later.

Copy link
Collaborator

@Xazin Xazin left a comment

Choose a reason for hiding this comment

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

Did a small review on the flutter changes.

Some more test cases would be very nice to have, eg. renaming/changing icon of child of child of parent, whilst parent is open, to ensure that the overview is updated accordingly.

@Xazin
Copy link
Collaborator

Xazin commented Jan 19, 2024

The analyzer is also failing, check the output of the CI.

@Xazin Xazin requested a review from LucasXu0 January 19, 2024 13:07
@LucasXu0
Copy link
Collaborator

@Jayaprakash-dev Why did you add the WorkspaceOverviewListenerIdManager? If you want to listen to current view changes, you can simply use a view listener or view bloc for that purpose.

@Jayaprakash-dev
Copy link
Contributor Author

@LucasXu0, The current implementation of the ViewListener class only notifies the first-level parent with the DidUpdateChildViews notification event. If an overview block is present in the top parent, it won't receive child view updates from deeper levels. It would be nice to have a separate event and folder notification specifically for the overview block. This enhancement would enable us to listen to child view changes and updates irrespective of their nesting level.

you can look at here how view changes are notified:
@manager_observer.rs:
notify_parent_view_did_change(folder.clone(), vec![view.parent_view_id]);

@ notify_child_views_changed function:

let parent_view_id = view_pb.parent_view_id.clone();
let mut payload = ChildViewUpdatePB {
    parent_view_id: view_pb.parent_view_id.clone(),
    ..Default::default()
};
.......
.......

send_notification(&parent_view_id, FolderNotification::DidUpdateChildViews)
  .payload(payload)
  .send();

Comment on lines +108 to +109
await tester.hoverOnPageName(firstLevelParentViewName);
await tester.renamePage('View 1');
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should never finish tests with "actions", only "expectations", otherwise you're just "doing actions" just to do them, without actually checking if what they did, worked as intended.

@@ -7,7 +7,7 @@ part 'cloud_env_test.g.dart';

/// Follow the guide on https://supabase.com/docs/guides/auth/social-login/auth-google to setup the auth provider.
///
@Envied(path: '.env.cloud.test')
@Envied(path: '.env.cloud')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Comment on lines +647 to +649
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
let mutex_folder = self.mutex_folder.clone().lock().as_ref();

Comment on lines +700 to +702
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
let mutex_folder = self.mutex_folder.clone().lock().as_ref();

Comment on lines +997 to +999
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mutex_folder = self.mutex_folder.clone();
let mutex_folder = mutex_folder.lock();
let mutex_folder = mutex_folder.as_ref();
let mutex_folder = self.mutex_folder.clone().lock().as_ref();

Comment on lines +34 to +35
let ffolder = folder.lock();
if let Some(folder) = ffolder.as_ref() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let ffolder = folder.lock();
if let Some(folder) = ffolder.as_ref() {
if let Some(folder) = folder.lock().as_ref() {

Comment on lines +35 to +73
if let Some(folder) = ffolder.as_ref() {
match value {
ViewChange::DidCreateView { view } => {
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Create,
manager,
folder,
);
}
notify_parent_view_did_change(folder, vec![view.parent_view_id]);
},
ViewChange::DidDeleteView { views } => {
for view in views {
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(view.clone()),
ChildViewChangeReason::Delete,
manager,
folder,
);
}
}
},
ViewChange::DidUpdate { view } => {
notify_view_did_change(view.clone());
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Update,
manager,
folder,
);
}
notify_parent_view_did_change(folder, vec![view.parent_view_id.clone()]);
},
};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if let Some(folder) = ffolder.as_ref() {
match value {
ViewChange::DidCreateView { view } => {
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Create,
manager,
folder,
);
}
notify_parent_view_did_change(folder, vec![view.parent_view_id]);
},
ViewChange::DidDeleteView { views } => {
for view in views {
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(view.clone()),
ChildViewChangeReason::Delete,
manager,
folder,
);
}
}
},
ViewChange::DidUpdate { view } => {
notify_view_did_change(view.clone());
if let Some(manager) = workspace_overview_listener_id_manager.upgrade() {
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Update,
manager,
folder,
);
}
notify_parent_view_did_change(folder, vec![view.parent_view_id.clone()]);
},
};
}
if let Some(workspace_listener) = workspace_overview_listener_id_manager.upgrade() {
if let Some(folder) = folder.lock().as_ref() {
match value {
ViewChange::DidCreateView { view } => {
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Create,
manager,
folder,
);
notify_parent_view_did_change(folder, vec![view.parent_view_id]);
},
ViewChange::DidDeleteView { views } => {
for view in views {
notify_child_views_changed(
view_pb_without_child_views(view.clone()),
ChildViewChangeReason::Delete,
manager,
folder,
);
}
},
ViewChange::DidUpdate { view } => {
notify_view_did_change(view.clone());
notify_child_views_changed(
view_pb_without_child_views(Arc::new(view.clone())),
ChildViewChangeReason::Update,
manager,
folder,
);
notify_parent_view_did_change(folder, vec![view.parent_view_id.clone()]);
},
};
}
}

@Xazin Xazin requested a review from appflowy January 29, 2024 21:03
@Jayaprakash-dev
Copy link
Contributor Author

Hey @Xazin, I've made quite a mess in the code file and inadvertently messed up the git files in my local repo. So, I've submitted a new PR #4629 that can be a replacement for this one.

If that works for you, I'll close this one.

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.

None yet

3 participants