david-swift / Memorize

Study flashcards in a native GNOME app
https://flathub.org/apps/io.github.david_swift.Flashcards
GNU General Public License v3.0
95 stars 9 forks source link

Add search for flashcards #34

Closed konstantintutsch closed 8 months ago

konstantintutsch commented 8 months ago

Steps

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 commented 8 months ago

@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?

david-swift commented 8 months ago

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 commented 8 months ago

SearchEntry() applied: screen recording

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

konstantintutsch commented 8 months ago

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

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

david-swift commented 8 months ago

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 774bde388dedf5825ff867a63a81ab7bd8395767.

Thank you so much for working on this!

david-swift commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

konstantintutsch commented 8 months ago

Search only working for the back of flashcards

konstantintutsch commented 8 months ago

This should be it

david-swift commented 8 months ago

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

konstantintutsch commented 8 months ago

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

david-swift commented 8 months ago

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 commented 8 months ago

I agree. This change makes it less confusing. 👌