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

Add search for flashcards #34

Merged
merged 21 commits into from
Mar 27, 2024
Merged

Conversation

konstantintutsch
Copy link
Contributor

@konstantintutsch konstantintutsch commented Mar 23, 2024

Steps

  • Add your name or username and a link to your GitHub profile into the Contributors.md file.
  • Build the project on your machine. If it does not compile, fix the errors.
  • Describe the purpose and approach of your pull request below.
  • Submit the pull request. Thank you very much for your contribution!

Purpose

Editing/fixing flashcards in large sets is a pain. A search for the content on the front or back of the flashcard fixes the issue.

Approach

If clause in var flashcards: View { ForEach() } in EditView.swift

This is my first time writing in Swift, don't expect the cleanest code. Review definitely necessary 😬

@konstantintutsch
Copy link
Contributor Author

konstantintutsch commented Mar 23, 2024

@david-swift I've used the EntryRow() function for inputting the search query.

I'm not sure if that is the most fitting UI element for search. Seems to be intended for less changing inputs, like flashcards, rather than often changing inputs.

Your thoughts on that?

var flashcards: View {
ForEach(.init(set.flashcards.indices)) { index in
if set.flashcards[safe: index] != nil {
if let flashcard = set.flashcards[safe: index],
flashcard != nil &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is responsible for an error:

warning: comparing non-optional value of type 'Flashcard' to 'nil' always returns true

It could be changed back to

set.flashcards[safe: index] != nil &&

I'm not sure if it can be left out. I'm unwrapping the flashcard one line above, so this check should be obsolete now, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, the if let flashcard = set.flashcards[safe: index] assigns a nonoptional value to flashcard so flashcard cannot be nil (because of its nonoptional type) where you are checking it. So you could remove the flashcard != nil check.

@konstantintutsch konstantintutsch marked this pull request as ready for review March 23, 2024 15:18
@david-swift
Copy link
Owner

Thank you so much 🙏😀

I think it makes more sense to filter the array containing the flashcards itself (similar to the line here for filtering sets) because one can change the order based on how close the content actually is. We could convert the provided example into a function and use it in both cases.

Instead of using an entry row, we could add a search button that opens a search entry (similar to the one in the sidebar) to the left of the toolbar in the editing dialog (and make Ctrl + F run the same action).

@konstantintutsch
Copy link
Contributor Author

konstantintutsch commented Mar 24, 2024

SearchEntry() applied: screen recording

Edit: .keyboardShortcut("f".ctrl()) only works with MenuButton() or Button() Extension.

@konstantintutsch
Copy link
Contributor Author

konstantintutsch commented Mar 24, 2024

I think the set filtering should be rebranded to set searching to have a consistent wording.
And also …

  • Filtering: x has to be a
  • Searching: x, y and/or z are similar to a

The Menu > Filter button could also be moved next to the Add Set button so it's easier to find. But that could also be overwhelming. Searching for sets is probably far less common and the option can stay hidden.

Bildschirmfoto vom 2024-03-24 11-08-12

Move func search() from struct FlashcardsSet
Apply func score() from struct FlashcardsSet to struct Flashcard

Create protocol SearchScore for func sortScore() to sort both
[FlashcardsSet] and [Flashcard]

!! Does not work in practice !!
@david-swift
Copy link
Owner

david-swift commented Mar 24, 2024

Edit: .keyboardShortcut("f".ctrl()) only works with MenuButton() or Button() Extension.

It doesn't matter whether a function is defined directly on the type or in an extension for the type, the result is exactly the same. We can use the same code for searching sets in the sidebar and check whether the edit mode is active, if yes open the search bar in the edit view, otherwise in the sidebar. I can implement this.

I want to keep the button for searching sets in the menu as it is, I think, a very uncommon action, especially after #10 is implemented. I agree that set filtering should be renamed to searching.

Concerning your last comment, I took a look at your code and updated it so that it works now. See 774bde3.

Thank you so much for working on this!

@david-swift
Copy link
Owner

As you are learning Swift, here some technical information about the previous commit.

The problem you were facing is described here. The solution in this case were generics.

You might also want to take a look at type extensions in Swift.

@konstantintutsch
Copy link
Contributor Author

Ah, okay. Thanks for compiling this information. Will definitely be helpful!

As someone who mainly programs in C, it's really great to know that a language takes care of things like this 😅

@konstantintutsch
Copy link
Contributor Author

Sorry for the force push, didn't see the variable renames and did a --fixup HEAD~1 instead of --fixup HEAD 😬

@konstantintutsch
Copy link
Contributor Author

Search only working for the back of flashcards

@konstantintutsch konstantintutsch marked this pull request as draft March 24, 2024 17:15
@konstantintutsch konstantintutsch marked this pull request as ready for review March 24, 2024 17:24
@konstantintutsch
Copy link
Contributor Author

This should be it

@david-swift
Copy link
Owner

david-swift commented Mar 24, 2024

Not sure whether suchen or durchsuchen is better - maybe rather durchsuchen (for both sets and cards)?

@konstantintutsch
Copy link
Contributor Author

I think suchen is the correct word. Durchsuchen is colloquial and emphasizing of suchen.

@david-swift
Copy link
Owner

david-swift commented Mar 26, 2024

I changed my opinion. I think searching should not change the order of the flashcards as this is more confusing than simply hiding the irrelevant flashcards and keeping the order. What do you think about this implementation, @konstantintutsch?

@konstantintutsch
Copy link
Contributor Author

I agree. This change makes it less confusing. 👌

Copy link
Owner

@david-swift david-swift left a comment

Choose a reason for hiding this comment

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

Thank you!

@david-swift david-swift merged commit 8394fc0 into david-swift:main Mar 27, 2024
1 check passed
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.

2 participants