Am 31. Oktober 2015 02:52:01 MEZ, schrieb Matthew Brush notifications@github.com:
[...] so unless there are actual problems I don't see an evidence to
keep the off default.
I mentioned a number of issues with it in the mailing list thread linked from the PR you made to disable it by default.
I don't think we should cram this in just because of string freeze, especially with the open issues, the UI issues, and that we will be releasing again in a few months. I'm not opposed to the feature, I'm opposed to the implementation (re-using an existing feature to do something else, something unexpected of the existing feature, as well as the infobar without a checkbox that can be hidden from the user without ever seeing it).
Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/672#issuecomment-152685983
Carrying this discussion to the ML.
Can you elaborate on the issues you see?
As to the UI. It's important that we get any UI for 1.26, therefore we had to manage before string freeze. We can introduce a better UI fir 1.27. I'm not opposed to a check box but would be more effort. And it would presumably checked by default anyway, no?
On 15-10-31 02:08 AM, Thomas Martitz wrote:
Am 31. Oktober 2015 02:52:01 MEZ, schrieb Matthew Brush notifications@github.com:
[...] so unless there are actual problems I don't see an evidence to
keep the off default.
I mentioned a number of issues with it in the mailing list thread linked from the PR you made to disable it by default.
I don't think we should cram this in just because of string freeze, especially with the open issues, the UI issues, and that we will be releasing again in a few months. I'm not opposed to the feature, I'm opposed to the implementation (re-using an existing feature to do something else, something unexpected of the existing feature, as well as the infobar without a checkbox that can be hidden from the user without ever seeing it).
Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/672#issuecomment-152685983
Carrying this discussion to the ML.
Can you elaborate on the issues you see?
Mostly the stuff I mentioned in the previous mailing list thread on this subject. The semantics of the Reload feature have changed and no longer does reload mean "drop everything about this file and load it again" it means, "store another copy of this file in RAM and load it again". I feel like it's not a good idea to re-use the File->Reload feature like this, and under not uncommon scenarios, Geany is no longer "lightweight" with respect to RAM usage.
To give an example, if someone opened a big log file and Reloads it, or if they have many files from version control open and switch branches, every reload will "leak" the size of each document worth of memory, making Geany use much more RAM than before. Scintilla's undo mechanism just isn't designed to efficiently handle the case of the entire document being replaced. The only workaround to regain all the memory wasted by Geany is to close all the files and start over.
I think the new preference is fine, I just don't think it ought to be enabled by default, or else it ought to be associated with a new (Edit, not File) action altogether as opposed to changing the semantics of the existing feature as everyone is used to.
As to the UI. It's important that we get any UI for 1.26, therefore we had to manage before string freeze. [...]
Why is it important to rush it in before 1.26? One of the points of the accelerated release cycle is so that people would stop wanting to cram stuff in right before release. It's only a few months until next release, so there's no need to keep trying to rush in controversial stuff.
Cheers, Matthew Brush
Am 31.10.2015 um 18:59 schrieb Matthew Brush:
On 15-10-31 02:08 AM, Thomas Martitz wrote:
Am 31. Oktober 2015 02:52:01 MEZ, schrieb Matthew Brush notifications@github.com:
[...] so unless there are actual problems I don't see an evidence to
keep the off default.
I mentioned a number of issues with it in the mailing list thread linked from the PR you made to disable it by default.
I don't think we should cram this in just because of string freeze, especially with the open issues, the UI issues, and that we will be releasing again in a few months. I'm not opposed to the feature, I'm opposed to the implementation (re-using an existing feature to do something else, something unexpected of the existing feature, as well as the infobar without a checkbox that can be hidden from the user without ever seeing it).
Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/672#issuecomment-152685983
Carrying this discussion to the ML.
Can you elaborate on the issues you see?
Mostly the stuff I mentioned in the previous mailing list thread on this subject. The semantics of the Reload feature have changed and no longer does reload mean "drop everything about this file and load it again" it means, "store another copy of this file in RAM and load it again". I feel like it's not a good idea to re-use the File->Reload feature like this, and under not uncommon scenarios, Geany is no longer "lightweight" with respect to RAM usage.
Reload barely means "replace the buffer with the current file's content", it's not connected to the undo history. And it still does that.
You can disable that feature. What's your problem?
To give an example, if someone opened a big log file and Reloads it, or if they have many files from version control open and switch branches, every reload will "leak" the size of each document worth of memory, making Geany use much more RAM than before. Scintilla's undo mechanism just isn't designed to efficiently handle the case of the entire document being replaced. The only workaround to regain all the memory wasted by Geany is to close all the files and start over.
So you are concerned about very large files. I understand that. But that's really an edge case, and I don't feel that everybody else should suffer for edge cases. Especially not because Geany is an designed to be an editor, not a very-large-file viewer.
The various preferences are exactly for that, to make up for such edge cases.
BTW, I think we only store the undo on reload if the file has actually changed since it was opened. So for simply viewing dumps there shouldn't be an increased memory footprint:
/* We only add an undo-reload action if the document has actually changed. * At the time of writing, this condition is moot because sci_set_text * generates an undo action even when the text hasn't really changed, so * actions_count is always greater than zero. In the future this might change. * It's arguable whether we should add an undo-reload action unconditionally, * especially since it's possible (if unlikely) that there had only * been "invisible" changes to the document, such as changes in encoding and * EOL mode, but for the time being that's how we roll. */
I think the new preference is fine, I just don't think it ought to be enabled by default, or else it ought to be associated with a new (Edit, not File) action altogether as opposed to changing the semantics of the existing feature as everyone is used to.
As to the UI. It's important that we get any UI for 1.26, therefore we had to manage before string freeze. [...]
Why is it important to rush it in before 1.26? One of the points of the accelerated release cycle is so that people would stop wanting to cram stuff in right before release. It's only a few months until next release, so there's no need to keep trying to rush in controversial stuff.
The feature was targeted at 1.25 already. The feature itself hasn't changed (you didn't do anything to optimize it either). We simply created a UI to inform about it. It wasn't rushed either. The PR was open for more than two weeks and was ignored.
Best regards.
On 15-10-31 11:54 AM, Thomas Martitz wrote:
Am 31.10.2015 um 18:59 schrieb Matthew Brush:
On 15-10-31 02:08 AM, Thomas Martitz wrote:
Am 31. Oktober 2015 02:52:01 MEZ, schrieb Matthew Brush notifications@github.com:
[...] so unless there are actual problems I don't see an evidence to
keep the off default.
I mentioned a number of issues with it in the mailing list thread linked from the PR you made to disable it by default.
I don't think we should cram this in just because of string freeze, especially with the open issues, the UI issues, and that we will be releasing again in a few months. I'm not opposed to the feature, I'm opposed to the implementation (re-using an existing feature to do something else, something unexpected of the existing feature, as well as the infobar without a checkbox that can be hidden from the user without ever seeing it).
Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/672#issuecomment-152685983
Carrying this discussion to the ML.
Can you elaborate on the issues you see?
Mostly the stuff I mentioned in the previous mailing list thread on this subject. The semantics of the Reload feature have changed and no longer does reload mean "drop everything about this file and load it again" it means, "store another copy of this file in RAM and load it again". I feel like it's not a good idea to re-use the File->Reload feature like this, and under not uncommon scenarios, Geany is no longer "lightweight" with respect to RAM usage.
Reload barely means "replace the buffer with the current file's content", it's not connected to the undo history. And it still does that.
Semantically Reload should do exactly what Close and then Opening the file again would do. We've changed the meaning of an existing feature to do something different (replace text from current disk file).
You can disable that feature. What's your problem?
I don't think it should be enabled by default.
To give an example, if someone opened a big log file and Reloads it, or if they have many files from version control open and switch branches, every reload will "leak" the size of each document worth of memory, making Geany use much more RAM than before. Scintilla's undo mechanism just isn't designed to efficiently handle the case of the entire document being replaced. The only workaround to regain all the memory wasted by Geany is to close all the files and start over.
So you are concerned about very large files. I understand that. But that's really an edge case, and I don't feel that everybody else should suffer for edge cases. Especially not because Geany is an designed to be an editor, not a very-large-file viewer.
Large files or many files. If someone has a huge project with many files open and changes Git branches, then for each of the documents, each time this is done, it will store a copy of the document in memory.
The various preferences are exactly for that, to make up for such edge cases.
The change is the edge case, the existing behaviour which has been in place for almost 10 years is the "normal" case. I agree this should be a various preference (or an edit action), I just don't think it should be enabled by default, changing the existing (common) meaning of Reload.
BTW, I think we only store the undo on reload if the file has actually changed since it was opened. So for simply viewing dumps there shouldn't be an increased memory footprint:
/* We only add an undo-reload action if the documenthas actually changed. * At the time of writing, this condition is moot because sci_set_text * generates an undo action even when the text hasn't really changed, so * actions_count is always greater than zero. In the future this might change. * It's arguable whether we should add an undo-reload action unconditionally, * especially since it's possible (if unlikely) that there had only * been "invisible" changes to the document, such as changes in encoding and * EOL mode, but for the time being that's how we roll. */
If that's the case, and we somehow tell if the file changed, then we shouldn't be prompting the user or anything either.
I think the new preference is fine, I just don't think it ought to be enabled by default, or else it ought to be associated with a new (Edit, not File) action altogether as opposed to changing the semantics of the existing feature as everyone is used to.
As to the UI. It's important that we get any UI for 1.26, therefore we had to manage before string freeze. [...]
Why is it important to rush it in before 1.26? One of the points of the accelerated release cycle is so that people would stop wanting to cram stuff in right before release. It's only a few months until next release, so there's no need to keep trying to rush in controversial stuff.
The feature was targeted at 1.25 already. The feature itself hasn't changed (you didn't do anything to optimize it either). We simply created a UI to inform about it. It wasn't rushed either. The PR was open for more than two weeks and was ignored.
I raised these same issues already, but I guess nobody remembers (except the mailing list archive and Github comments). When I say rushed, I refer to your comment "String freeze is tomorrow so please merge" and the fact that I raised these issues on the PR and yet it was merged before I even had a chance to respond to the comments added.
Cheers, Matthew Brush