When using New Project/Open project dialogs or the recent project files submenu, there's no need to confirm closing the project (so long as the dialogs can be cancelled before closing the session). It's unlikely those actions are done by accident. I've kept confirmation for opening a project file from the command line, in case it was a glob mistake. OSX confirmation is kept because I'm not sure what the OSX code is for and also can't test.
* Make Open Project dialog cancellable without closing existing session * Opening recent projects: Don't ask whether to close current project * Make New Project dialog cancellable without closing existing session (this one needed some refactoring)
These 3 are in separate commits, probably best to view commits separately. This also probably makes #2171 unnecessary. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2178
-- Commit Summary --
* Make Open Project dialog cancellable without closing existing session * Make New Project dialog cancellable without closing existing session * Opening recent projects: Don't ask whether to close current project
-- File Changes --
M src/project.c (91) M src/ui_utils.c (3)
-- Patch Links --
https://github.com/geany/geany/pull/2178.patch https://github.com/geany/geany/pull/2178.diff
I haven't reviewed the code yet, but :+1: in concept.
Even without the improvements regarding being able to cancel the dialogs and leave the old project open (nice BTW), I've always felt asking to confirm closing the project was unnecessary as it only happens in response user actions and (AFAIK) is totally non-destructive.
I've kept confirmation for opening a project file from the command line, in case it was a glob mistake.
I have never considered command-line user, can you give an example of how something bad could happen in this case?
+1 the intention, hand holding dialogs like this quickly become annoying.
Good idea to leave the command line dialog, although IIRC the `xxx.geany` file has to be the first one to open as a project, not as a keyfile, so you would be unlucky.
Not tested yet, please don't merge until somebody has a chance to confirm on each of OSX (since you mentioned it) and Linux and Windows.
IIRC the xxx.geany file has to be the first one to open as a project, not as a keyfile
Just tested, it doesn't have to be the first filename.
can you give an example of how something bad could happen
`geany *` would open some filenames in the current session, then open a project file, then open the remaining filenames under the project. There could even be more than one project file in the list of filenames. We should probably Requiring the project filename to be first on the command-line and ask whether to open the project, handling the no project open case. But asking whether to close the current project is still better than no prompt at all.
please don't merge until somebody has a chance to confirm on each of OSX (since you mentioned it) and Linux and Windows.
This pull does not change the OSX-specific code, that will still work. The Linux code is also used on Windows, so I have tested it and tested the native Windows dialog path too.
I plan to merge this unless there's a good reason not to - see my comments.
Nobody else seems to have tested it yet.
@elextr I'm going to merge this soon as 2 developers agree with the change and I have tested all changed code paths. I don't think we should require pulls to be tested by a reviewer, that will inevitably cause good work to stay in PR purgatory instead of being merged. Also the easiest way for most people to test changes is when they're in master, and this also means the changes get real-world testing when the user is actually working on real code.
@ntrel et al,
Well in the past I would have loudly complained about things being merged without testing by someone other than the author because I used Geany most of the day most days, and simply couldn't afford instability.
But my day to day needs have changed and I have had to move away from Geany as it simply does not provide the functionality I need any more. So I am no longer regularly testing anything without specifically starting Geany and that may cause delays in doing so, as I posted on a @kugel- PR.
So that means I am also less concerned if Geany breaks, but, as I said somewhere, all of us miss stuff when testing our own because we _know_ how its meant to work, other people try silly things and break it, and so its important that things get third party testing, especially as the project has no effort or process available for verifying stability at release time. So I am still concerned about everybody merging their own without checking and testing.
On the other hand, you are right that currently non-controversial PRs do tend to sit for a long time because nobody other than the author is interested in them, and so nobody tests them.
So the project needs to come to some solution.
To get the ball rolling I suggest that if, within a reasonable period, say two weeks, nobody objects or says to wait, and none of the usual contributors explicitly suggests third party testing or changes are needed, then PRs that don't make big changes should be allowed to be merged by their authors (or if they are external PRs by a sponsor with merge rights).
As you suggested, for bigger PRs, in particular that change behaviour without any option to turn them off, two explicit approvals are needed (and silence isn't approval) and third party testing is also needed. If they can be turned off (plugins or options) then only one approval, and testing that it doesn't break anything if its turned off, should be enough.
My reasoning for this is that developers often think their way is the best and only sensible way for the application to work, without appreciating other user points of view. So if changes are going to be forced on everyone, and in particular if they break workflows, they need to meet a higher bar than ones that can be turned off. That is why I have been constantly requesting options to turn off changes that make significant alterations to workflows. Default to on or off is a case by case decision.
And I strongly emphasise that testing doesn't have to be Geany developers, anybody that wants to should be encouraged to test changes.
@ntrel,
Sorry (after the fact) to hijack your PR :cry: feel free to copy the above post to a separate issue it you think its worth it.
This PR I would suggest has the ability to work around it, as you say there are cancel buttons on the remaining dialogs.
As for OSX testing, I only mentioned it because you mentioned you have been working near OSX specific code, so you might have accidentally changed some of its preconditions, even if you didn't actually touch the OSX stuff itself, and I havn't had the chance to check (and might not recognise it even if I did).
IIRC the xxx.geany file has to be the first one to open as a project, not as a keyfile
Just tested, it doesn't have to be the first filename.
Ok, manual needs fixing then [last row of the table](https://www.geany.org/manual/current/index.html#command-line-options).
...my day to day needs have changed and I have had to move away from Geany as it simply does not provide the functionality I need any more.
In some way, Geany not meeting your needs ties back to the way PRs are merged and/or how things improve. I was working on a plugin that in theory would've probably provided the features you can't get from Geany (I assume you're referring to deeper C++ integration/IDE features/smarts), but it would have required me to make some larger changes to Geany to do it (ft-plugins). IIRC at the time I felt it would be too much effort to get the changes into Geany, not for technical reasons but because of the onerous PR/discussion process. I'm not implying/suggesting anything here, just pointing out the relation.
I assume you're referring to deeper C++ integration/IDE features/smarts
Yeah, I deliberately didn't state it because I didn't want to make it look like I was blaming anyone.
Its possible that clang (or other language server) integration would provide enough capability to meet the needs, but as you say there are fundamental changes necessary to support it ... continued on #2283
This needs to be merged to master as no one has got round to testing it other than me. In master it will get thorough testing. People using master should not expect high stability.
People using master should not expect high stability.
Unfortunately this probably means nobody will use master so it won't get tested.
But I don't have any magical solution when there are no testers.
@b4n Unless there's a release soon, can I merge this to master so people can test it?
Merged #2178 into master.
github-comments@lists.geany.org