Skip to content

Add option to disable search including variables.#3415

Closed
dicene wants to merge 1 commit intoMudlet:developmentfrom
dicene:variableSearchToggle
Closed

Add option to disable search including variables.#3415
dicene wants to merge 1 commit intoMudlet:developmentfrom
dicene:variableSearchToggle

Conversation

@dicene
Copy link
Copy Markdown
Contributor

@dicene dicene commented Mar 10, 2020

Brief overview of PR changes/additions

Adds a toggle in the Editor tab of Preferences that controls whether or not variables are included in the search functionality in the Editor.

image

Motivation for adding to Mudlet

Over the course of many years using Mudlet, I'm not sure that I have searched variables intentionally more than a single time. However, due to the large amounts of data I keep saved in various tables in Lua, searching through the variables causes Mudlet to lag for several seconds any time I want to search.

Other info (issues closed, discussion etc)

I'm inclined to think searching variables should be disabled by default, seeing as it has almost never been useful for me, and I've seen/heard other people mention that it was unnecessary for them, and just served to inflate search times and disincentivizes using it.

That said, I'm not certain that all the primary devs of Mudlet will agree with me on that, so I've set it to default to ON.

@dicene dicene requested a review from a team as a code owner March 10, 2020 12:53
@dicene dicene requested a review from a team March 10, 2020 12:53
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Mar 10, 2020

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 10, 2020

I agree with the default to off - but could it be more context-sensitive and go into the search options we already have? It'll be easier to find and change there.

@dicene
Copy link
Copy Markdown
Contributor Author

dicene commented Mar 10, 2020

I agree with the default to off - but could it be more context-sensitive and go into the search options we already have? It'll be easier to find and change there.

I wasn't aware that we HAD any search options already. I don't see them in Profile Preferences. I was a little unsure about whether Editor was the right place for it, but I was initially just thinking of it as options for Edbee, but IMO the tab is broader than that, including anything in the entire Script Editor itself.

@dicene
Copy link
Copy Markdown
Contributor Author

dicene commented Mar 10, 2020

Actually, I think I've found you what you mean. Although, I think it's a bad sign as far as visibility goes if there are options that even someone that uses the program extensively(and contributes to it) wasn't aware of it in the first place...

This guy?
image

I suppose we COULD put it there instead, but I'm inclined to think that the number of people aware of the Case Sensitivity option in the first place is probably very low. I don't think that icon does a good job communicating that there are options to set. A "..." or gear icon would be more informative, I think. Additionally, the Case Sensitivity option is NOT saved between sessions, and I think that this particular option is appropriate for saving in the profile.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 10, 2020

It should be saved between sessions for sure, it'd be quite annoying if it didn't.

As for the icon - I don't mind changing it to something more intuitive. Ask around to see if nobody else knew what it did?

@demonnic
Copy link
Copy Markdown
Member

I knew, but should not be the yardstick by which this is measured, as I know about a lot of things that other people are surprised to hear.

@SlySven
Copy link
Copy Markdown
Member

SlySven commented Mar 15, 2020

Well I put the option there because I'd been using Qt Creator for years and that is where they put a number of options:
image
image

image
image
image
image

Have a look at (QAction) dlgTriggerEditor::mpAction_searchCaseSensitive (and the uncoded for yet mpAction_searchWholeWords and mpAction_searchRegExp) - that option is NOT preserved between sessions - as it is wasn't worth the extra coding that would take IMVHO. Also you might want to consider (void) dlgTriggerEditor::createSearchOptionIcon() as you will need to create some more icons - I suggest that you call the option ignore variables and looks like the symbol we use for "Variable" with a line through them - that graphic will need to be combined with the one for case sensitive (:/icons/searchOptions-caseSensitive.png) for the case for both options being enabled and then each of them will need some white-space on one side so that the combination is the same width as either of them individually (or the red or grey magnifying glass binoculars icons) - so the remainder of the QLineEdit does not get offset by different amounts when different options are set/unset.

Hell, we even put in the clear button that shows up when there is something in the QLineEdit on the right hand side, just like Qt Creator:
image

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 26, 2020

@dicene Did you want to finish this up? It's a popular request - otherwise I'm happy to complete it

@dicene
Copy link
Copy Markdown
Contributor Author

dicene commented Mar 28, 2020

Requested changes made in another branch, this is now superceded by #3543.

@dicene dicene closed this Mar 28, 2020
@dicene dicene deleted the variableSearchToggle branch April 4, 2020 16:38
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.

4 participants