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

Interactively autoincrement list when commenting #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

viktorasl
Copy link
Contributor

Hello! 👋 It's my first try to contribute to this amazing tool. To make it even more efficient to construct lists I've worked on interactive auto-increment (seen the request GitHawkApp/GitHawk#1692).

Currently it does not do anything clever, just checks if a single new line text insertion occurs and then analyzes that text line. The work is done in textView(_:shouldChangeTextIn:replacementString:) which means that pasted text is ignored but that could be changed probably without much effort as the actual logic is decoupled.

In short what was done:

  • Observes changes made in text view and autoincrements both numbered and unordered
    lists if list constructon is detected.
  • Preserves text insertion text attributes
  • Sets correct text selection after autoincrement text is inserted

Next in feature polishing:

  • Preserve the indentation allowing same level list auto-increment

* Observes changes made in text view and autoincrements both numbered and unordered
lists if list constructon is detected.
* Preserves text insertion text attributes
* Sets correct text selection after autoincrement text is inserted
@SD10
Copy link
Member

SD10 commented Mar 31, 2018

@viktorasl Thanks for submitting this PR ❤️ I haven't dug into your code, but I think this functionality may be better targeted as an extension to GitHawk itself. But I could be wrong, just passing by 😇

@rnystrom
Copy link
Member

Love this idea! But also agree it’d be better off in GitHawk itself because we shouldn’t have any actual features in this library.

Sent with GitHawk

@viktorasl
Copy link
Contributor Author

Oh, ok, I'll move the feature to GitHawk but not sure how I can implement there without modifying MessageViewController as I have to modify textView(_:shouldChangeTextIn:replacementString:) method. Any ideas how I can track what's being edited and where?

@SD10
Copy link
Member

SD10 commented Apr 1, 2018

@viktorasl IIRC there is an abstraction MessageTextViewListener that receives information from textView(_:shouldChangeTextIn:replacementString:). You just have to add your object conforming to this protocol to the array of listeners in the MessageTextView. We then iterate the array inside of the shouldChangeText delegate method.

@viktorasl
Copy link
Contributor Author

Yeah, but the replacement string is not passed to the listeners

Sent with GitHawk

@SD10
Copy link
Member

SD10 commented Apr 1, 2018

@viktorasl You can submit an initial PR making it available? It’s useful information. I almost did this before when needing to fix a bug

Sent with GitHawk

@viktorasl viktorasl closed this Apr 1, 2018
@viktorasl viktorasl reopened this Apr 1, 2018
@viktorasl
Copy link
Contributor Author

viktorasl commented Apr 1, 2018

Sorry for close and reopen, was testing GitHawk accessibility, accidently closed the issue.

Didn’t fully get the idea: shall I add the additional parameter in listeners metod to be able to pass replacement string in this repo?

Sent with GitHawk

@SD10
Copy link
Member

SD10 commented Apr 1, 2018

Yes @viktorasl that sounds 👌 to me

@viktorasl
Copy link
Contributor Author

viktorasl commented Apr 1, 2018

I faced some problems:

  • textView(_:shouldChangeTextIn:replacementString:) method has to return false if text was adjusted because it now adds actually typed new line where the cursor is placed. (e.g - A\n becomes not - A\n- but - A\n- \n).
  • Don't actually know how to workaround. I have an idea to change willChangeRange(textView: MessageTextView, to range: NSRange) so that it would return Bool and then textView(_:shouldChangeTextIn:replacementString:) result to false if at least one listener returns false. But it's quite fragile as generally result depends on a single listener response (if it's false).

Any ideas to solve those issues? I'm quite stuck not finding the balanced solution.

@viktorasl
Copy link
Contributor Author

More to that: willChangeRange(textView: MessageTextView, to range: NSRange) won't reflect actual change range if text is adjusted. Hmm, well this is a tricky one.

@viktorasl
Copy link
Contributor Author

Hey, long time no see!
Can I do something to push this feature or should this just be closed?

@BasThomas
Copy link
Collaborator

cc @rnystrom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants