Vs find dialog style #588
Vs find dialog style #588
Conversation
[CTRL]-F, [CTRL]-H, and [CTRL]-[SHIFT]-F behavior is changed copying VS IDE style. 1) Only one dialog can be displayed at the time. 2) Marked text is automatically loaded as text to find
added Search dialogs keypress event handler gboolean on_search_key_press_event(GtkWidget *widget, GdkEventKey *ev, gpointer user_data)
|
You have commits for different things in the one PR, please separate them into different PRs. |
|
Sorry; I've added both mods together because they are all VS features. |
|
You can leave them together if you wish, but although the first simple enhancement of copy filename to clipboard is likely to be fairly uncontroversial the later parts look like they would be more controversial, so they may delay the original change. |
|
I might add that as most of the Geany devs do not use windows, you might be better off if you actually described what the changes are, and what their benefits are, rather than "like VS" since that might be meaningless to non-VS users and also their response might be "who cares if its the same as VS, we don't use it". |
|
Hi there, please believe me; it is not laziness, I just can't find a neat way to separate one PR in 2 PR. Please 1st let me say I'm a guy familiar with VS but I'm using Geany exclusively under Linux, I find these mods very useful when using Geany under Linux but surely they will also be enjoyed by people using Geany under Windows. Mod 1) Before the patch: b) Many times the action of calling the find dialog boxes is triggered by some text already marked in the current file; unfortunately Geany does not not always automatically copy the marked text as the text to look for ("Search for" field) in the invoked find dialog box. After the patch b) when opening any find dialog box the marked text in the file (if any) is "always automatically" copied to the "Search for" field of the invoked find dialog box avoiding unnecessary copy&paste actions. Mod 2) Please let me know if you guys need more detail about these mods. |
|
According to TravisCI this PR breaks the build. |
It sounds weird to be referring to an absolute path, would the comment be like this? /* This function is defined in C:\Users\John Smith\Projects\TheProject\Source\SomeModule.cc */
void foo(void); |
|
I agree with @elextr this should be split into two PRs. For example I'd probably merge the notebook tab menu item one myself once it's fixed, but I probably won't touch it while it has all the unrelated stuff in the same PR. It's pretty easy to split them: $ git checkout master
$ git checkout -b notebook-copy-filename
$ git cherry-pick 656fbe657f176bda934cb5bc52f6715882ab7233Something like that ought to make a new branch for the copy filename thing, then you can push that to your Github repo and make a new pull request. |
|
Guys, I'm not using git on my builds, I just patch and rebuild the corresponding Debian package, the one I'm using "every" day; the mods work. The mods are very simple but useful (at least for me) if you guys consider they have some value then feel free to alter the formatting, or the code, and test the thing. If you guys think they are not worth it just reject the PR. Best, |
IIUC this is intentional, they are modal dialogs, there is no reason for example that you should not be able to find-in-files, and then perform replacements alternately. So I am against changing this. Better to be tidy and close the dialog when you are finished with it, click close or Alt+c.
Works for me, if it doesn't for you please identify when and raise an issue/PR just for that fix. [...]
As @codebrainz says, this is a reasonable improvement, but it needs to be separated. |
You need to at least submit the patch against the current development version, it basically does not compile due to an undefined variable, thats a pretty obvious problem, but who knows what subtle errors may occur by applying a patch from an old version. |
I find your way of working highly confusing, You can find-in-files then open among the found files the files you like and next perform a replacement but never alternatively. VS implements a highly mature IDE that has been tested by millions of developers during many (really many) years. I agree with them; find dialog boxes should be displayed one at the time. Just my 2 cents. BTW How many years have you been coding using "find in file" and "find&replace" alternatively?
No it doesn't work; please look at the code before you say things that are not true. the former code clearly shows that the selection is set only if the dialog is not visible; what IMHO is nuts.
Best, |
This mod is based in Geany v1.25 from Debian repository.
It attaches to the tab menu a new option ("Copy Full Path") which copy to the clipboard the full path of the open file.
|
I think the missing "page" variable issue is already solved. You guys have recently changed (v1.25) the prototype of and that led to the error; Best, |
it avoids a harmless assert
There's a coding convention we follow and if we just merged any old style that each contributor prefers the code would be even more of a mess. If you can't even bother to follow simple coding conventions, what are the chances that you paid attention to important stuff in your patches? |
I agree. But considering my patch is so simple, probably I expected talking more about the usefulness (or not) of the thing before than talking about a coding convention.
You are mixing bananas with oranges; code quality/usefulness is one thing and coding convention is a different matter. I have contributed code in many places in my life and I cannot consider strict coding conventions for a patch that has less than 100 lines. In other places they usually mention the coding convention when you become a frequent contributor, but never as the first comment when you just sent your first 90 lines of code.. Anyway, the patch works in 2.15 now, you can test it if you like. If you decide to promote the change and you really need some specific code convention trim please let me know. Best, |
It shows overall attention to detail and it's all described in detail in the HACKING file to avoid this kind of non-productive discussion.
So because it's less than 100 lines, someone else should clean up the code for you? We're all volunteers, if you want to increase the likelihood of someone merging the change you want, you should make it as easy as possible.
I was doing a review of the changes, and it's the very first thing that stuck out to me and needs to be fixed before it can be merged, just as much as any missing symbols or other bugs. |
You are dead wrong; I do not "want" any change; I just thought of Geany's community; it is my way to contribute to a project I consider useful; BTW I get Geany the way I want in 5 minutes recompiling a Debian package w/o having to read "funny" comments on the number of spaces between an "if" and "("....
My friend.. this is not the kind of sentence that avoids escalating an argument... specially when in my last post I wrote: If you decide to promote the change and you really need some specific code convention trim please let me know probably you just didn't read it. Best, |
Oh, you want the search string to continue to follow changes in selection. No, to use your term, that is "nuts", you can't edit anything that involves changing the selection and keep the search term, which is especially bad when the user has set the search term to be a regular expression not just a simple string. It is also "nuts" to apply arbitrary restrictions to what "search like" dialogs a user can have open, especially as find in files is completely unrelated to the current file search and replace dialogs. And finally it is not acceptable to force changes that affect user workflows like this "because its the way VS works" without providing an option for them to turn the features off. Copy filename to clipboard could be handy, and is likely to be acceptable as an individual PR. |
Oh, you are obviously not an every day coder using Geany.
i.e. this would be the classic case when parsing nested functions and we do not have intellisense available.
What???
Again you prove here that you are talking about what you do not use regularly.
Please.. who's the one that is forcing anything?? I would agree though that a configuration parameter switching the dialog boxes behavior could be a good idea for people like you used to cumbersome ways to move around an IDE. If you like that idea then you have homework. Best, |
|
@ppatpat I think you took a very unfortunate confrontational approach which will eventually lead to not merging your patches not because they aren't useful but because others will get tired of the pointless discussion like above. Here are a few points related to my experience with how to get my patches merged: Do a good patch marketing. It's your task to convince others your patch is useful - the fact that the particular feature isn't in Geany yet probably means others don't care much and you must convince them it's something worth having in Geany. Clearly describe what problem you are trying to solve with your patches - what the state was before and how your patch improves things. Do this preferably directly in the commit message. Until your fifth comment in this thread it wasn't clear to me what you are trying to achieve apart from "doing it the VS way" (I have never used visual studio and the fact that another editor does things differently isn't a good argument). By the way Gedit works the way you suggest too which is a closer Geany's competitor. Save the reviewer's/maintainer's time. It's the maintainer's time which is expensive, not yours. There are typically tens of patches waiting for review and the maintainer will pick those which are as easy to review as possible. You significantly increase the likelihood your patches get reviewed if they are nice-looking - don't submit a half-baked crap for review. IMO your patches are really messy. First, they don't contain any commit messages. Describe what your are trying to achieve as suggested above and the way you are implementing it. Patches like "Update notebook.c" or "Update search.c" really don't help the reviewer to identify what you perform in the patch. Patches should describe the particular feature you are trying to implement, not files which got changed. What might help is looking at already merged patches to see what form they should have. Second, do a self-review of the code you publish - don't leave commented-out code in the patch, try to follow the style you see in the code as much as possible (even if you feel your style is the "right" one) Take the "the reviewer is always right" approach (unless it's something important you disagree with). You can start arguing your coding style is better, that the review process should change, that other open source projects do something in a different ("better") way etc. but these things just move the discussion somewhere else and delay the patch acceptance. Concentrate on the important thing - to get the patch in and don't lose your focus. Typically it's also faster to update the patches than endless discussions. For instance, does the reviewer say you should drop curly braces around single-line if's? Simply do it. You can start a discussion that you have been using curly braces in this case because it's safer, that it's considered a best-practice and that other projects do it this way too but you just start fragmenting the discussion and instead of getting your patch in you get stuck on something which is irrelevant to the patch and the patch doesn't get merged. (If you want to change the coding style, open a new ticket or suggest something on the mailing list). And really, really don't start getting unfriendly towards others: accusing long-term Geany developers of "not being an every day coder using Geany", telling people they are "mixing bananas with oranges" when they just say your code should be cleaned up a bit before it can be reviewed and so on is plain stupid. This kind of "I know best, others are idiots" approach has never helped anything and nobody wants to work with people with such an attitude. Be determined to finish what you have started. I must say I was quite fascinated by your reaction here:
What do you want then? In my opinion submitting a patch for review means "I want to get this change into Geany and I will continue working on the patches based on the reviewer's comments". If you don't want to get the patch into Geany, it's pointless to submit it. And be prepared that during the review process there will be further comments requiring additional work - for my patches it's often something like 40% time spent on the initial set of patches and 60% to finally get them in. If you are not willing to incorporate the reviewer's comments, nobody else will do the work for you. Finally, be patient. I sometimes have problems with this myself but there's a limited time people involved in Geany can spend so your patches can be unreviewed for quite some time. tl;dr By the way, I have never seen the problem the patch tries to solve myself because I use the "Hide the find dialog" option from Preferences->General->Miscellaneous so it's always hidden after searching so there are never 2 diallogs at the same time and the marked text is always preloaded because Ctrl+F leads to opening the dialog. One more note: I don't want to turn this into some kind of flame war so I'm probably not going to continue in the discussion if it looks similarly to what preceded this post. |
|
@techee I appreciate you taking the time. It seems to me your motivations for publishing your code are different than mine. As I've said before I use Geany, I improved a couple of things and I just wanted to make them public. That's my way to say "thank you" to the community that makes Geany. Making my changes available to the public is all I need here; I do not really need to see them "accepted" nor "implemented" either.
Sorry; I do not agree.
I have been reviewing code myself many times and this is my top-down approach.
As you can see in this case we began by rushing to the formal aspects of the thing even skipping the fundamental point 1. Finally as a reviewer I try to be assertive;
That option closes the find dialog box after a match. That is not really a very handy feature. The find dialog should remain open while not purposely closed (that's what serious IDEs do).
The text preloading only happens when the find dialog is not previously displayed. Please read my description of "concatenated searches".
Sure; me neither. Best, |
|
On 15-07-30 01:02 AM, ppatpat wrote:
If you haven't bothered to make them acceptable, and you don't care if If you just want to toss around ideas, use the mailing list, or if you
Everyone's time is important, and your patches are really messy. None of
Thank you for wasting my time on pure crap patches that I don't
It was so simple and non-controversial a patch, that even though it was
If you just do this before opening a pull request, there's no silly
Sorry, we have pull requests from people with VASTLY different skill What I was trying to say, translated from Canadian to American is "Why
Why do you talk like this? Please speak plainly. If you want to call me
Again, the patch I reviewed, even though it was muddled in an unrelated
To that end, I'm going to close this pull request. If you actually want |
This is not true; there was an obvious issue with v1.25 the one that was immediately solved. I have also said that if the patch was considered for inclusion I was willing to parse the coding convention issues; then please stick to the facts.
Please do not change my words; I do care about the inclusion of my code but that is not my main goal here. Can you see the difference?
Tossing around ideas is good when you haven't coded a single line; I have a fully working mod; can you see the difference?
I appreciate if you do not use personal adjectives; keep it professional please.
Enough my friend; you are loosing your temper; that's not good. This conversation ends here. Best, |
Changes the "Find" dialog box behavior like VS
[CTRL]-F,
[CTRL]-H,
[CTRL]-[SHIFT]-F
behavior is changed mimicking VS IDE style.
a) Only one dialog can be displayed at the time.
b) Marked text is automatically loaded as text to find
Attaches to the tab menu an option giving the full path of the open file.
Very handy when writing/commenting about code.