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

Adds hidden preference 'always_set_from_selected' (default: FALSE) [...] #2695

Closed
wants to merge 0 commits into from
Closed

Conversation

etkaar
Copy link
Contributor

@etkaar etkaar commented Dec 20, 2020

See issue #2693.

Adds hidden preference 'always_set_from_selected' (default: FALSE) which updates search phrase with selected text when search/replace dialog is reopened (means, if the dialog is still opened). Default behaviour is not changed.

@elextr
Copy link
Member

elextr commented Dec 20, 2020

Where is the preference set? It should be on the find dialog not just read from the config file.

There has been no discussion about it applying to replace.

Needs to be documented in the manual (no it can't be left, it will be forgotten, answering your question from #2693).

@etkaar
Copy link
Contributor Author

etkaar commented Dec 20, 2020

Where is the preference set? It should be on the find dialog not just read from the config file.

Since I don't think there is a need to often turn on/off this preference, I don't think we should extend the search/replace dialog with this setting, so I choosed the way of making it a hidden preference. It can be found and easily toggled at Edit > Preferences > Various, so there is no need to manually edit any config files:

Screenshot from 2020-12-20 23 41 54

There has been no discussion about it applying to replace.

I don't think users which want to use this preference want a different behaviour between search/replace, so the setting applies to both of them. However, it would be no problem to make two (hidden) preferences if needed.

Needs to be documented in the manual (no it can't be left, it will be forgotten, answering your question from #2693).

Okay, then I will create a new PL later with the second commit which updates the manual.

@elextr
Copy link
Member

elextr commented Dec 20, 2020

We are very much trying to not dump things in various, so if its a preference it should be in a normal part of the preferences, probably editor->features which has plenty of room.

Don't make a new PR, just push changes to this one.

@codebrainz
Copy link
Member

We are very much trying to not dump things in various...

For what it's worth, this is not shared by all core developers; personally I feel that the Preferences dialog already has loads of options and that it's better for preferences that are obscure or rarely used to be dumped into the Various preferences tab. In any case, I don't feel strongly, I just mention so it's clear this is not some kind of agreed upon policy. IMO, leave it to the person willing to make the PR, it can always be moved later if it turns out people are using it a lot for some reason or having trouble finding it.

@elextr
Copy link
Member

elextr commented Dec 21, 2020

IMO, leave it to the person willing to make the PR, it can always be moved later if it turns out people are using it a lot for some reason or having trouble finding it.

Sure, but if we don't ask we don't know if the OP is happy to do it.

@elextr
Copy link
Member

elextr commented Dec 21, 2020

Seems there may have been a misunderstanding, I was saying that there needs to be an option if the search is automatically updated on selection, not if it requires user action like pressing ctrl-f.

The name of the option always_set_from_selected suggested it was automatic, but having had a chance to look at the PR code it appears to only happen on ctrl-f (or whatever the search shortcut is set to).

If @etkaar confirms thats the case then I agree with @codebrainz it doesn't need an option, just mentioning in the manual.

@elextr
Copy link
Member

elextr commented Dec 21, 2020

@etkaar also just noticed you made the changes in master, not in a branch, when you update can you please make the changes in a branch, that way committers can switch back from your changes to master or another set of changes easily.

@etkaar
Copy link
Contributor Author

etkaar commented Dec 21, 2020

The name of the option always_set_from_selected suggested it was automatic, but having had a chance to look at the PR code it appears to only happen on ctrl-f (or whatever the search shortcut is set to).

Correct, the update of "Search phrase:" with the selected text only happens on the "reopen" event of the already visible dialog initiated by using the shortcut or menu Search > Find... or Search > Replace...

@etkaar also just noticed you made the changes in master, not in a branch, when you update can you please make the changes in a branch, that way committers can switch back from your changes to master or another set of changes easily.

Thanks for the note, I will try that!

@elextr
Copy link
Member

elextr commented Dec 21, 2020

@etkaar did you make your query about the docs on a commit that you have now deleted?

Anyway, edit geany.txt, geany.html is generated from it by rest2html but is kept in the repository so its available to repository (and tarball) users without needing rest2html. It would be appreciated if you could re-generate it, but its not required, but of course that adds to the mergers work and likely slows the merge.

@etkaar
Copy link
Contributor Author

etkaar commented Dec 21, 2020

@elextr Yes, the next time I should better use Comment and not delete it. I deleted it because I in the meanwhile found out that geany.html is automatically updated when I edit geany.txt. I made a new Pull Request (#2697) and think now all is correct (but the .html is not transferred to GitHub because it is not part of the GitHub repository).

@elextr
Copy link
Member

elextr commented Dec 21, 2020

(but the .html is not transferred to GitHub because it is not part of the GitHub repository).

Ahh yeah, you are right, we took it out, so only the geany.txt is needed.

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.

None yet

3 participants