CodeEdit icon indicating copy to clipboard operation
CodeEdit copied to clipboard

Settings Search

Open luah5 opened this issue 2 years ago • 20 comments

Description

This PR adds support for searching settings in CE's settings, it works very similar to the way System Settings searches.

Related Issues

  • close #1309

Checklist

  • [x] I read and understood the contributing guide as well as the code of conduct
  • [x] The issues this PR addresses are related to each other
  • [x] My changes generate no new warnings
  • [x] My code builds and runs on my machine
  • [x] My changes are all related to the related issue above
  • [x] I documented my code
  • [x] Code is clean
  • [x] Highlighted text of search
  • [x] Fix grouping issue

Screenshots

https://github.com/CodeEditApp/CodeEdit/assets/128280019/118ee588-9edb-4b73-a7f6-f70aa6f2d5e6

Extra notes

I've updated the docs for adding a new setting so that it appears in the search results

luah5 avatar Jun 12 '23 20:06 luah5

@luah5 :

https://github.com/CodeEditApp/CodeEdit/assets/20476002/022af962-8234-4c14-81fa-43fa78bb1537

  1. It would be possible for everyone in the same place to group them.
  2. When clicked, on the text it should open the place where it is which happens, but you don't understand where that text is, it would be possible to make it flash or something similar to how android does when you search for something in the search bar settings.

Angelk90 avatar Jun 13 '23 11:06 Angelk90

image

You'll notice that:

  1. The text is aligned with the parent item's label
  2. The child item itself is selectable
  3. There is only one parent item, in yours there is a parent for every child
  4. The search term is white while the rest of the text has an opacity (maybe .secondaryLabelColor)

austincondiff avatar Jun 13 '23 15:06 austincondiff

@luah5 : When selecting an element I don't think @austincondiff wants the whole group to be selected, at least macOS Ventura doesn't do that, it only selects the single element.

https://github.com/CodeEditApp/CodeEdit/assets/20476002/aa43b639-505a-456e-bed6-27508ca6d991

For point 2) I think it is useful to understand where the property is, then you need to hear what others think ask @austincondiff.

Angelk90 avatar Jun 13 '23 19:06 Angelk90

@Angelk90 I'd agree with out that it should probably flash or jump out at you in some way. System settings doesn't do this though so we might save this for another PR. Regardless, my guess is It is selecting all the related items because they all point to the same location, so they are all selected. We need to think of a workaround to this so only the item that is clicked is selected.

austincondiff avatar Jun 13 '23 20:06 austincondiff

@luah5 : The thing that the whole group remains selected does not convince me.

https://github.com/CodeEditApp/CodeEdit/assets/20476002/0b80e01e-52a4-47e9-b53c-e92472fd17c9

Angelk90 avatar Jun 15 '23 09:06 Angelk90

@luah5 When there is no search text input, the menu item color needs to be primary. It looks like it is defaulting to secondary. It should only be secondary when there is text input into the search field.

austincondiff avatar Jun 18 '23 20:06 austincondiff

@austincondiff , @luah5 : When the possibility of having CE in multiple languages will be enabled later and there will be all the translations, how will this system work at the moment?

Angelk90 avatar Jun 19 '23 09:06 Angelk90

Nice work! I noticed some issues:

  • Tab selection state isn't kept

Could you show a video, does the tab selection need to be kept across sessions with UserDefaults?

  • I should be able to go through the search results by pressing the up and down arrow keys

Re implementing that feature makes the search results group again

  • It is difficult to search for settings because the search keywords don't match the "real" labels of the settings

Fixed.

This is a good base, but I think it needs improvements. I strongly suggest using settings labels instead of variable names as keywords. This is what will need to happen anyway. Not doing that now is a bit pointless, as the work from this PR would be removed later on. I would also like to avoid complaints from users about search not being accurate / predictable

Fixed.

luah5 avatar Jun 24 '23 15:06 luah5

@luah5 Several things:

  1. The text size is too small. Please use the original text size.
  2. The default color of main navigation items is too light.
  3. Result items text should line up with main items text. Give it a little more left spacing.

austincondiff avatar Jul 07 '23 19:07 austincondiff

@austincondiff

luah5 Several things:

  1. The text size is too small. Please use the original text size.

Fixed, I'm not sure what happened though.

  1. The default color of main navigation items is too light.
CodeEdit settings

New: Right, Old: Left

I can't see a difference, but if you were referring to the text, I have fixed that.

  1. Result items text should line up with main items text. Give it a little more left spacing.

Fixed in the latest commit

CodeEdit settings

luah5 avatar Jul 10 '23 11:07 luah5

Font size and text color are still incorrect.

austincondiff avatar Jul 10 '23 13:07 austincondiff

@luah5 :

https://github.com/CodeEditApp/CodeEdit/assets/20476002/1977dff2-2b49-4c52-8a92-30da7579e4b0

Angelk90 avatar Jul 10 '23 13:07 Angelk90

@luah5 Several things:

  1. The text size is too small. Please use the original text size.
  2. The default color of main navigation items is too light.
  3. Result items text should line up with main items text. Give it a little more left spacing.
  1. I just realized this is because of the sidebar size system setting. Disregard this.
  2. This looks like it was addressed. Thanks!

austincondiff avatar Jul 10 '23 13:07 austincondiff

@luah5 :

Latest commit version, as you can see when I click on an element then it loses focus because the focus is on the input field.

Problem with font or size, looks weird.

mac os ventura.

https://github.com/CodeEditApp/CodeEdit/assets/20476002/71c3fcd7-84e9-48a4-a36a-591c1595f8b4

Angelk90 avatar Jul 10 '23 13:07 Angelk90

@luah5 :

Latest commit version, as you can see when I click on an element then it loses focus because the focus is on the input field.

Problem with font or size, looks weird.

mac os ventura.

Registrazione.schermo.2023-07-10.alle.15.50.34.mov

Fixed :)

luah5 avatar Jul 10 '23 20:07 luah5

@luah5 : Is it normal that it loses focus when I click?

https://github.com/CodeEditApp/CodeEdit/assets/20476002/81eae85e-7cb8-424b-b61f-fcee50311afd

Angelk90 avatar Jul 11 '23 08:07 Angelk90

@luah5 : Is it normal that it loses focus when I click?

https://github.com/CodeEditApp/CodeEdit/assets/20476002/81eae85e-7cb8-424b-b61f-fcee50311afd

That is not normal, and I cannot reproduce it, what version of macOs ventura are you on?

luah5 avatar Jul 11 '23 08:07 luah5

@luah5 : Ventura 13.2.1

Angelk90 avatar Jul 11 '23 10:07 Angelk90

I genuinely have no clue what's happening maybe a macOS bug. As you can see in this video the same issue doesn't occur on my machine, also check you are on the latest version.

https://github.com/CodeEditApp/CodeEdit/assets/128280019/16821687-9fcb-44b1-a65e-37bbed60f4b3

Thanks for the report @Angelk90, I appreciate it!

luah5 avatar Jul 11 '23 11:07 luah5

@austincondiff : Do you also have this problem?

Angelk90 avatar Jul 11 '23 15:07 Angelk90

Where are we on this PR? Is it ready to be approved? Is there anything pending revision?

austincondiff avatar Jul 18 '23 02:07 austincondiff

@austincondiff, I'm just waiting for it to be reviewed.

luah5 avatar Jul 22 '23 14:07 luah5

What is the status on this PR? We need to either finish it or close it. There is a lot of good work here so I'd rather push it over the finish line if we can.

austincondiff avatar Aug 16 '23 17:08 austincondiff

What is the status on this PR? We need to either finish it or close it. There is a lot of good work here so I'd rather push it over the finish line if we can.

Sorry for the recent inactivity, I'll try to get this finished with my limited time at the moment. Then this PR can be merged.

luah5 avatar Aug 19 '23 08:08 luah5

I would love to have this PR merged, could some people please review this PR?

luah5 avatar Aug 21 '23 08:08 luah5

@luah5 can you fix the conflict with CodeEdit.xcodeproj/project.pbxproj? Then it should be fine for re-review.

0xWDG avatar Sep 10 '23 17:09 0xWDG

@luah5 can you fix the conflict with CodeEdit.xcodeproj/project.pbxproj? Then it should be fine for re-review.

sure

luah5 avatar Sep 11 '23 17:09 luah5

just fixed the merge conflicts, if a reviewer has time, please review this PR thanks!

luah5 avatar Sep 11 '23 18:09 luah5

@luah5 FYI, shorthand_operator should not trigger if you use Text() + Text(). see https://github.com/realm/SwiftLint/issues/4936, https://github.com/realm/SwiftLint/issues/4953 for more information.

0xWDG avatar Sep 11 '23 19:09 0xWDG

In the interest of preserving the work that has been done and not letting this PR become stale, I merged this PR. For those that requested changes to this PR, feel free to open follow-up PR's for these changes.

austincondiff avatar Sep 30 '23 15:09 austincondiff