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: Add popups to start new conversations #76

Merged
merged 30 commits into from
Aug 24, 2022
Merged

Conversation

RemiBardon
Copy link
Member

The "Add member" popup:

Screenshot 2022-06-27 at 15 44 11

What we have now:

Screenshot 2022-06-27 at 18 51 12

Screenshot 2022-06-27 at 18 51 32

Screenshot 2022-06-27 at 18 52 03

Screenshot 2022-06-27 at 18 52 16

The "Create or join group" popup

Screenshot 2022-06-27 at 15 44 22

What we have now:

Nothing

@RemiBardon RemiBardon added the feature New feature to implement label Jun 27, 2022
@RemiBardon RemiBardon self-assigned this Jun 27, 2022
@RemiBardon RemiBardon marked this pull request as draft June 27, 2022 17:05
@RemiBardon
Copy link
Member Author

RemiBardon commented Jun 28, 2022

After a long discussion on Slack yesterday, we have come up with a better UI idea.

To avoid shifts, and to allow suggestions reusability, here is the layout we want to create:

Screenshot 2022-06-28 at 10 21 39

I made this quickly in SwiftUI, so it's not perfect, but you get the idea.

One major problem with using vanilla SwiftUI is that we loose out-of-the-box accessibility, keyboard navigation and support for text field tokens, which we'd like to use.

Today I'm looking at using native AppKit components, and making this view more reusable so we can use it in other places (everywhere we need to search for a chat address).

I will use this PR to document my process, and paste all the interesting links I found.

@RemiBardon
Copy link
Member Author

RemiBardon commented Jun 28, 2022

Useful links:

@RemiBardon
Copy link
Member Author

lucasderraugh's code automatically used the system selection style! In his video it looked rectangle, and on my macOS 12 it automatically switched to rounded rectangle, perfect 😍

From his video: Screenshot 2022-06-28 at 10 53 09

On my computer: Screenshot 2022-06-28 at 10 52 04

@RemiBardon
Copy link
Member Author

I'm having a hard time trying to wire the SuggestionsWindowController

@RemiBardon
Copy link
Member Author

RemiBardon commented Jun 28, 2022

Here is what I could get:

Screenshot 2022-06-28 at 22 26 35

✅ For now we can:

  • Select with arrow keys
  • Hit Return to change the text to the selected row content
  • Select a row by clicking on it (not sending the row to the text field)
  • Change the background, the corner radius, the position, the size… everything

✨ Also:

  • The list is a system component, so it benefits from native UI, features and accessibility
  • The suggestion window goes over the bounds of the current frame, which would not be possible to acheive if this wasn't its own window
  • Fully programmatic

📝 TODO:

  • Custom row content (from SwiftUI)
    • Entire view from SwiftUI
  • Refactoring/cleanup
  • Clicking on a row enters the text and closes the popup
  • Text field tokens (basic)
  • Custom text field tokens
  • TCA integration
  • Portability (prepare for different features)
  • Focusing the text field opens the window (if there is content to show), not on first character entered (optional, not the default behaviour on Finder)

It was a lot of work, for not much code, but I'm sure we can make this very useful and portable.

@RemiBardon
Copy link
Member Author

Oh! I'm thinking the entire list could be SwiftUI 💡

This would be far easier and more flexible than having an AppKit NSTableView with SwiftUI cells inside 👍🏼

We could even show something other than a list if we wanted (e.g. add a header/footer) 💡

@RemiBardon
Copy link
Member Author

I thought about this again, and I'm not sure it'll be easy to keep the keyboard navigation if the list comes from SwiftUI. Passing a binding to the selected row could do it, I'll try.

@RemiBardon
Copy link
Member Author

OK, I managed to get a lot further 👍🏼

With a custom VStack:

Screenshot 2022-06-29 at 15 10 45

Here, we loose the native benefits of List, but we gain full customization, and we are guaranteed that the UI will not changed with an OS update.
However, we also avoid the fact that (until macOS 13), all Lists are scrollable, even if you do some maths to make the list take exactly the size it needs.

With a List:

Screenshot 2022-06-29 at 15 11 00

  • I can't remove the background
  • The list is scrollable, even if it's fitted to the right size

@RemiBardon
Copy link
Member Author

I cleaned up the code a lot. I still have some cleanup to do, then I'll make it so tapping on a row selects it (+ hover style?).

After that I'll integrate TCA.

I should have done most of this by tonight.

@RemiBardon
Copy link
Member Author

I'll try to have a look at tokens tonight, and do the TCA wrapping tomorrow

@RemiBardon
Copy link
Member Author

After a short discussion on Slack yesterday, we decided I would look at tokens soon, but do the TCA wrapping first.

Here is what I came up with:

Screen.Recording.2022-06-30.at.11.28.03.mov

There are still minor things to fix, but it's going in the right direction!

@RemiBardon
Copy link
Member Author

Support for sections + loading from TCA environment:

Screen.Recording.2022-06-30.at.15.35.57.mov

@RemiBardon
Copy link
Member Author

I didn't try, but you should be able to navigate across sections (there is no reason this would not work).

@RemiBardon RemiBardon requested a review from nesium July 6, 2022 09:50
@nesium
Copy link
Contributor

nesium commented Jul 6, 2022

Phew, ok. Great that it works with the combination of TextAttachments and SwiftUI views and windows and what have you! There's a lot going on which took a great deal of experimentation and research. Well done!

Instead of moving on with this PR, I think we should first try to find the right abstractions for these components. In my opinion it would make sense to create separate new PRs where you pull over the respective code, possibly creating multiple stacked PRs with each component.

Here's my suggestion for what I think should be 4 components:

  1. A TCA-driven TextView
  2. A generic AutoSuggest reducer and environment
  3. A SwiftUI window View à la .alert() or .popover()
  4. A ContactSuggestionField that composes all of the first 3 parts

The benefit of splitting the functionality into smaller bits is that we can test each very easily and compose them into bigger components, like the actual ContactSuggestionField. They will also be easier to understand since they only deal with one thing and we can also configure them for our different use-cases. E.g. the contact field in the multi-user chat which probably wraps and grows up until to a certain number of lines. Or the sidebar in the same view which presents its search results inline instead of in a window (see below).

Screenshot 2022-07-06 at 19 22 33

Here are the features of each component:

1. TCA-driven TextView + Reducer

  • Configurable to a certain degree visually and functionally
    • Visually: Things like corner radius, the focus ring, etc.
    • Functionally: Maximum number of lines (before it scrolls vertically)
  • Can show a placeholder
  • Sends and receives text changes to and from ViewStore
  • Sends and receives selection changes to and from ViewStore
  • Its state might offer convenience methods to query the current selected word
  • Selects the next responder when Tab is pressed
  • Does not know anything about tokens it only deals with an NSAttributedString and a NSRange.

Thruth be told, we'll certainly need multiple versions of this. The field in the sidebar will probably wrap a NSSearchField. However everywhere where we need tokens we'll need to use a NSTextView as you've discovered. And there are at least two use-cases which look quite different: The contact picker in the multi-user chat and the picker for adding a new contact to the roster. In any event we could reuse the reducer.

2. AutoSuggest reducer and environment

  • The environment we have basically
  • The reducer is missing (like the one I sent you in the Gist)
  • The reducer accepts an action that contains a search query, then performs the search via its environment
  • Handles loading, error and empty states

2.1 AutoSuggest list

We could either provide a ready-made list which displays AutoSuggestSections or built it from scratch when needed. The ready-made list would…

  • Accept a view for its cells provided from outside
  • Bind its selection to the ViewStore
  • Handle loading, error and empty states
  • Its state offers convenience methods to manipulate the list selection

It would come with a tiny state wrapper:

@dynamicMemberLookup struct AutoSuggestListState<T: Hashable & Identifiable>: Equatable {
  var selection: T.ID?
  var state: AutoSuggestState<T>
  
  subscript<U>(dynamicMember keyPath: WritableKeyPath<AutoSuggestState<T>, U>) -> U {
    get { self.state[keyPath: keyPath] }
    set { self.state[keyPath: keyPath] = newValue }
  }
}

3. SwiftUI window

  • A custom container view in the same shape as .alert() or .popover()
  • Takes only a View for its content and a Bool Binding to determine if the window is presented or not
  • It would be fantastic if the window could be closed on click outside, but that could require serious hackery (e.g. swizzling NSApp.sendEvent). But maybe there is an easy solution?

4. ContactSuggestionField (or actually just a reducer)

  • It's a data-only component
  • The view is composed from scratch where we need it to handle the case where search results are either displayed inline or in a popover (window)
  • Composes the TCA-driven TextView reducer and AutoSuggest reducer
  • Feeds events into the respective reducer by providing relatively little glue code
  • Manipulates the focus when receiving up/down key events by using the convenience methods in the AutoSuggestList state
  • Closes the window when escape is pressed
  • Inserts token from list selection when return is pressed or an item in the list is clicked
  • Can be hard-coded to contacts for now, we can make it generic when we need it
  • Sets a Bool when it "thinks" that the window with results should be presented/dismissed but its only a hint since the list could be displayed inline

The usage could look like this:

struct AddContactScreen: View {
  let store: Store<ContactAutoSuggestState, ContactAutoSuggestAction>

  var body: some View {
    WithViewStore(self.store) { viewStore in
      TCATextView(self.store.scope(state: \.textView, action: ContactAutoSuggestAction.textView))
        .drawsFocusRing(true)
        .cornerRadius(8)
        .attachedWindow(isPresented: viewStore.binding(\.$isSuggesting)) {
          AutoSuggestList(store: self.store.scope(
            state: \.suggestionList,
            action: ContactAutoSuggestAction.autoSuggest
          )) { contact in
            HStack {
              Text(verbatim: contact.name).bold()
              Text("")
              Text(verbatim: contact.jid)
            }
          }
        }
    }
  }
}

and just a little more context…

struct TCATextViewState: Equatable {
  var text: NSAttributedString
  var selectedRange: NSRange?
}

enum TCATextViewAction: Equatable {
  case textDidChange(NSAttributedString)
  case selectionDidChange(NSRange)
  case keyboardEventReceived(KeyEvent)
}

public enum KeyEvent: Equatable {
  case up
  case down
  case newline
  case escape
}

enum Loadable<T: Equatable>: Equatable {
  case notRequested
  case loading(previous: T)
  case loaded(T)
  case error(EquatableError, previous: T)
}

struct AutoSuggestState<T: Hashable & Identifiable>: Equatable {
  var content: Loadable<[AutoSuggestSection<T>]>
  // …
}

@dynamicMemberLookup struct AutoSuggestListState<T: Hashable & Identifiable>: Equatable {
  var selection: T.ID?
  var state: AutoSuggestState<T>
  
  subscript<U>(dynamicMember keyPath: WritableKeyPath<AutoSuggestState<T>, U>) -> U {
    get { self.state[keyPath: keyPath] }
    set { self.state[keyPath: keyPath] = newValue }
  }
}

enum AutoSuggestAction<T: Hashable & Identifiable>: Equatable {
  case searchQueryChanged(String?)
  case itemSelected(T)
  case autoSuggestResponse(Result<[AutoSuggestSection<T>], EquatableError>)
  case retryButtonTapped
  // …
}

struct ContactAutoSuggestState: Equatable {
  var textView: TCATextViewState
  var suggestionList: AutoSuggestListState<Contact>
  @BindableState var isSuggesting = false
  // …
}

enum ContactAutoSuggestAction: BindableAction, Equatable {
  case textView(TCATextViewAction)
  case autoSuggest(AutoSuggestAction<Contact>)
  case binding(BindingAction<ContactAutoSuggestState>)
  // …
}

Regarding the behavior of the ContactSuggestionField, I think we're still a little fuzzy there. You can easily end up with orphaned text which wasn't converted into a token. The popover will then also not appear anymore. I think we should clarify with @valeriansaliou what the expected behavior is.

@valeriansaliou
Copy link
Member

@nesium providing more details on the expected behavior of the contact picker in this view:

  • You can either look for contacts from your contact list, or an arbitrary JID;
  • Pressing return on an entered JID that doesnt pass a JID format check regex should do nothing (do not accept, do not clear);
  • Whenever there's non-tokenized orphaned text in the text area, and the user starts the conversation anyway, then the orphaned text should be discarded, we'll only use the values that were tokenized as recipients for the DM (if 1 recipient) or MUC (if multiple recipients).

@RemiBardon
Copy link
Member Author

@nesium I had not seen your comment! I will read it now, but just know that I moved the suggestions to #78, so we can merge this quickly, and think about how to better implement suggestions in a longer time.

@RemiBardon
Copy link
Member Author

RemiBardon commented Jul 11, 2022

(I will answer here, even though the code has moved somewhere else.)

Thank you for this very long proposition/study 🤩 I agree with most of what you said, except the few things I noted down there 👇

I think It will require a lot of time to implement all of this, and I don't think it's the most required feature right now. I split this PR in two, so we can have a basic text field with only strings, which can be submitted only if the JID matches some known JID. Once we can start chats and all, I'll come back to #78.

Just typing what crosses my mind:

Does not know anything about tokens it only deals with an NSAttributedString and a NSRange.

NSAttributedString and NSRange are TextKit 1, and do not support most non-latin localizations (the ones with non-contiguous characters). We need to use NSTextRange instead, which is useless without its associated NSTextContentStorage. Therefore, we have to keep a reference to NSTextContentStorage, which is a reference type (with value semantics). If we have such a reference, we don't need to send events with data for selection and text changes, as the data already lives in the NSTextContentStorage.

enum Loadable<T: Equatable>: Equatable {
  case notRequested
- case loading(previous: T)
+ case loading(previous: T?)
  case loaded(T)
- case error(EquatableError, previous: T)
+ case error(EquatableError, previous: T?)
}

.loading and .error must store a T?, as data might have never been fetched successful.

@nesium
Copy link
Contributor

nesium commented Jul 11, 2022

The problem with reference types in a TCA state is that you cannot detect changes when you observe the store via a ViewStore.publisher. Old state and new state will always be the same if nothing else has changed since they both share the same reference. In the Crisp app I've handled this by manually creating a copy (COW) when modifying the NSAttributedString.

@RemiBardon
Copy link
Member Author

You're right, that's (one of the many reasons) why I don't like my current implementation 😕

(Did you know AttributedString is a struct?)

@RemiBardon
Copy link
Member Author

Functionally: Maximum number of lines (before it scrolls vertically)

@nesium Do we need such text field in multiline?

(I'm asking because we intercept the return key event, which prevents the user from typing a newline.)

@nesium
Copy link
Contributor

nesium commented Jul 13, 2022

Functionally: Maximum number of lines (before it scrolls vertically)

@nesium Do we need such text field in multiline?

(I'm asking because we intercept the return key event, which prevents the user from typing a newline.)

Answered in Slack (just for completeness).

@RemiBardon RemiBardon marked this pull request as ready for review July 14, 2022 18:07
@RemiBardon
Copy link
Member Author

@nesium I had to rebase, and by rebasing it updated swift-case-paths, which resolved the problem I had with reducers. This PR is now ready 🙂

@RemiBardon RemiBardon marked this pull request as ready for review August 22, 2022 13:01
Copy link
Contributor

@nesium nesium left a comment

Choose a reason for hiding this comment

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

Two things:

  1. Please add the SearchSuggestionsFeaturePreview to the Makefile so that it gets built with the others.
  2. Can we get rid of the fake validation/delays to not waste time unneccessarily during testing?

@RemiBardon
Copy link
Member Author

Please add the SearchSuggestionsFeaturePreview to the Makefile

The code from this PR was originally in another PR, SearchSuggestionsFeaturePreview should not have been there at all. I removed it.

@RemiBardon RemiBardon merged commit afa6e7e into master Aug 24, 2022
@RemiBardon RemiBardon deleted the conversation-popups branch August 24, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants