[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@ntrel You seem to have misunderstood the point of my objection. The problem was the process, merging a change before others had the chance to assess it, not that there was a technical problem with the PR. Since as I noted I had not had time to see its contents I could not be commenting on the content. > Taken together though, my points justify the merge IMO. This was not a crash problem or a sudden regression, it was an inconvenience that Geany users had been living with for a long time, there was no need to hurry a merge without giving others time to assess it. There is no release planned for tomorrow, so there is no rush to merge to enable distribution of the change to users. Therefore nothing justifies a quick merge. > There are no other cases. I thought we agreed that wrapping should not always > be on, and this does not conflict with wrapping. Again, I was not taking about the technical content of the PR, sorry if that was not clear. As I said above I was talking generally about the process, "cases" meant applying your sole judgement to the "obviousness" and complexity of the change to justify a quick merge for this and future PRs. Having had a chance to look at the changes, why not put `reset_msgwin` in `ui_utils` and call it from both `build` and `search`, or is there some technical reason that the order of the calls needs to be different in build? If there is it probably should be commented so someone doesn't reorder them in the future. > Reverting is not the end of the world. Agree totally, thats a very mature and sensible approach, reverting is not a criticism, it should be thought of in the same way as deleting a line of code if it needs to be changed, but some members of the Geany project seem to have a horror of it :-) -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2054522950 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
Each of your rebuttals is valid on its own. Taken together though, my points justify the merge IMO. > It may be right in this case, but it may not be in another. There are no other cases. I thought we agreed that wrapping should not always be on, and this does not conflict with wrapping. > Having someone else review your changes is part of a cooperative project, not > merging before they have a chance. Reverting is not the end of the world. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2054060580 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
> It was open for a week Availability of our contributors is limited, a week is nowhere near time that guarantees people have time to notice, and review it. Most only have weekends, but if they are busy one weekend they have missed your PR. > you didn't object to the change after seeing it I suggested an alternative approach and we are looking at that on another PR, that is clearly not approval of this change. > it's an obvious improvement. Because we are looking at an alternative I have not even looked at the implementation of this, so "obvious" is purely your own judgement. It may be right in this case, but it may not be in another. Having someone else review your changes is part of a cooperative project, not merging before they have a chance. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2054023539 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@elextr It was open for a week, you didn't object to the change after seeing it, it's an obvious improvement. There's no reason why either treeview should be wider than necessary. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2054005309 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@ntrel, where does anybody say they have tested or reviewed this? -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2053823635 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
Merged #3816 into master. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#event-12457768225 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
> Ok, true, which suggests having a menu item to turn it on and off quickly and > easily, not a preference. Yes. > Or maybe > [wrap](https://docs.gtk.org/gtk3/property.CellRendererText.wrap-mode.html) > could be turned on. I've tried that - see #3834. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2053618054 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
> all I was saying was that wrapping is not always an improvement. Ok, true, which suggests having a menu item to turn it on and off quickly and easily, not a preference. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2052987288 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@elextr Of course we should support wrapping, all I was saying was that wrapping is not always an improvement. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2052135159 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
> Often when doing Find in Files I'm not interested in any matches on long > lines (e.g. text files), I'm only interested in matches in code which are > almost always short lines. Wrapping would just add more vertical noise (in > this case). See your use-case and raise you a different use-case. But then if searching documentation where modern markups tend to use single line paragraphs edited with wrap on it would be very useful as most of the matching line would be off screen and the horizontal scrolling would be horrible :-) If you don't want to add wrap just don't do it, I am only asking, but don't try to justify it with easily refutable use-cases. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2043985269 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@elextr BTW this applies to search as well. Often when doing Find in Files I'm not interested in any matches on long lines (e.g. text files), I'm only interested in matches in code which are almost always short lines. Wrapping would just add more vertical noise. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2039815569 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
I'm not so sure there is a "reason not to use wrap", you have made a speculation, but have you tried it? In Vscode wrapped error messages (G++) are still the crap the compiler gives, but at least there is no need to scroll side to side to try to figure out what its whinging about, its possible to read it all. Thats not to say this PR isn't an improvement, I just was looking to see if there might be more improvement available. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2039601327 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
@elextr That would probably be a nice option to support, but in the case of very long lines the option could be annoying and cause a lot of extra scrolling if you are looking for something near the start of a line. So given that there are reasons not to use wrap, I think we should reset the width. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2039538006 You are receiving this because you are subscribed to this thread. Message ID:
[Github-comments] Re: [geany/geany] [msgwin] Reset compiler/messages treeview width at start of build/search (PR #3816)
Or maybe [wrap](https://docs.gtk.org/gtk3/property.CellRendererText.wrap-mode.html) could be turned on. -- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/3816#issuecomment-2039491302 You are receiving this because you are subscribed to this thread. Message ID:
