Configuration Option to Prevent Window Raise on 'File Modified Externally'

[jlsantiago0]

I use mouse follow focus and would like to see a configuration option that disables the raising of the window if 'File Modified Externally' happens on a window that gets focus but is not raised 'e.g. is behind another window' and hasnt been clicked on. The window should only raise in that case if it is clicked.

[eteran]

I'll see what I can do

[eteran]

So I'm going to keep looking into this, but there is a feature which gets you halfway there (but at a cost). You can disable the feature entirely With:

Preferences -> Default Settings -> Warnings -> File Modified Externally

Not ideal since you'll NEVER get alerted, but better than it firing every time you move the mouse around ;-).

[jlsantiago0]

Hi @eteran I made an attempt at solving this issue:

See jlsantiago0@8b13ece

It currently tracks whether the current window is modified and the text cursor position the last time File Modified Externally was checked. And if the file gets externally modified but the local window has not been modified and the cursor position hasnt changed, then it wont display the File Modified Externally QMessageBox until the user changes the cursor position. Which has a high probability when the user clicks on the text window.

This is not a perfect solution, but it works pretty good for me so far.

I tried to find a way to work around the actual problem that opening the QMessageBox when File Modified Externally happens also raises the whole application window. But I couldnt figure out anyway to do that. That would be the ideal solution. I dont mind the pop-up box iteself, i just cant have the window raising forward and interrupt what i am currently doing in another window.

ideally I could tie it to the user clicking on the application anywhere instead of just moving the cursor. But I couldnt figure out where/how to find out when that happens. Then i wouldnt have to tie to the cursor position. just clicking anywhere for instance on a menu, on the title bar, on the scroll handle, even moving the mouse wheel on the application window.

I was wondering if you would accept a PR of this kind of change. I would need to make a configuration preference for it. I wanted to make sure you would accept something like this before I try and figure out how the configuration system works and how to add a configuration parameter for this feature.

Also if you could point me to a way that i could tell if the user interacts with the application in another way than it just getting focus, we could tie it to that instead and it would be a better solution.

Anyway. Thanks for this application. If you dont want to be bothered with this feature, I can maintain it on my own in my repo. Especially if there isnt anyone else that would like to have this feature.

[eteran]

@jlsantiago0 Not only am I happy to look at your PR, but I also love the idea of having more contributors to the project :-)

I haven't looked at your branch quite yet, but your description sounds reasonable at first glance. I'm usually happy to tuck divergences from how legacy nedit works behind a setting. (And honestly if NG is just "different" because of Qt-isms already, maybe if your approach makes it different, but better than current NG it can be the default...)

So yes, please make a PR, I'd love to look at it!

[eteran]

OK, took a quick look and the concept seems pretty sound. We should probably add some setting for it, and perhaps it's possible to detect if the window manager has mouse follow focus enabled? If so, we could possibly make it a bit automagical

[eteran]

Maybe, if we're lucky, we can detect a difference in focus reason?

https://doc.qt.io/qt-5/qt.html#FocusReason-enum

[jlsantiago0]

OK. I will see if FocusReason gives us any useful information.

[jlsantiago0]

Doesnt seem like we can use the FocusReason. But i think i see where i can get the information about someone clicking on the text window, using the mouse wheel in the text window, and moving the sliders. So that can be used to improve this further.

Let me make those improvements and I will make a PR. Is it difficult to add an appilcation configuration option for this feature? Perhaps you can point me to the useful bits.

Seems like it should go under the DefaultSettings->Warnings list.

[eteran]

It's not too difficult at all. I would start with it being entirely in the config.ini and then we'll worry about the UI bits.

Basically, it's a bit more complex than it should be because the design evolved slowly from where nedit was to where NG is, but here's the gist:

You add a new bool variable to Settings.cpp and Settings.h in the Settings library (there's plenty of examples there).

Then you add lines to loadPreferences, importSettings, and savePreferences.

save and load are fairly straightforward, you're simply getting a value and casting for a load, and simply setting a value for save. "import" is SLIGHTLY more complex in that it's like a load, except that if it's not found, it will default to the existing value (this is used to allow multiple files to be loaded and they can "layer" over each other.

So in summary:

load:

// read nedit.autoSave as a bool, defaulting to true from config.ini
autoSave = settings.value(tr("nedit.autoSave"), true).toBool();

save:

// save to nedit.autoSave in config.ini
settings.setValue(tr("nedit.autoSave"), autoSave);

import:

// same as load, but preserves the existing value if not present
autoSave = settings.value(tr("nedit.autoSave"), autoSave).toBool();

From there... we can (and sometimes do, I need to be more consistent) just include "Settings.h" in the appropriate file and then direclty read/write the setting as needed.

But we often have an extra layer of abstraction especially if it is going to be set by the UI. So we'll probably want to do it.

In the main src directory, there are getters and setters for all primary settings in Preferences.cpp / Preferences.h. This has served as a place to put logic for "this should happen when the setting changes" and similar.

Which tends to look like this (following the same autoSave example):

void SetPrefAutoSave(bool state) {
	if (Settings::autoSave != state) {
		PrefsHaveChanged = true;
	}
	Settings::autoSave = state;
}

bool GetPrefAutoSave() {
	return Settings::autoSave;
}

So... there's not a "lot", but it's not completely trivial.

let me know if you have any questions.

Once you have this working, I can help with the UI bits :-)

[jlsantiago0]

Perfect. Thank you for the write up. It is very helpful. It will likely take me a little time.

[jlsantiago0]

Hi @eteran

I have the feature itself implemented and it seems to be working very well.

See jlsantiago0@51e9a5c

It currently operates as the original behavior. Which will pop-up the File Modified Externally dialog as soon as it recognizes that the current file has been modified externally.

The next step is to add configuration options.

I envision 3 checkboxes in the:
Default Settings:
Warnings:
[x] Files Modified Externally
[x] Check Modified Files Contents
[x] Always
[x] When Modified
[x] When User Interacts

The When Modified and When User Interacts only matters when Always is unchecked.

You can test the different behavior by setting the 3 parameters in const Feature167Config feature167Config(true, true, true, isWindowModified_, userInteractionDetected_); in DocumentWidget::checkForChangesToFile() in src/DocumentWidget.cpp. The intent is more obvious if you have your window manager set to 'Mouse Follow Focus, Do not Raise.' Mode. This mode instructs the window manager to provide the focus including the keyboard focus is the window that the mouse pointer is above. And if you move the mouse pointer above the application window of another app, that window gets the focus, but it does not raise the focused window unless you click on it. The intent is that if we set [ ] Always, [x] WhenModified, [x] WhenUserInteracts . That we can modify the file externally and have a bunch of overlapped windows with the nedit window not responding to the external change unless the local version is was previously modified or until the user clicks on the window.

My next step is to figure out how to add this to the configuration per your earlier instructions and to remove all the debug. Let know know if you like the current implementation or if i need to change anything.

[jlsantiago0]

OK. I just added the configuration management for the 3 new options:

warnAlways
warnWhenLocalMods
warnWhenUserInteracts

[jlsantiago0]

OK. So the feature is implemented and the configuration options are created and being used for implementation.

jlsantiago0@22a06ba

The next step is adding the configuration to the UI. Looking at that now to see if i can figure it out.

[jlsantiago0]

I submitted a pull request. #326 . Thank you for considering my PR. The code for this project is well organized btw. I was able to find what i needed without too much hastle.

[eteran]

@jlsantiago0 fantastic. Life is a little bit on the busy side right now, so it may take a little bit before I give it a close look. But rest assured, I am very interested in getting improvements in!

Thanks!

[eteran]

@jlsantiago0 OK, I took a quick look and it generally looks great, you even did the UI parts :-) !

I have one question though. Is the intention for

warnAlways
warnWhenLocalMods
warnWhenUserInteracts

and the implied warnNever to be mututally exclusive, as in, it is only meaningful to pick one?

If so, we can probably use a group to make the UI elements act as a radio select. I think the independent settings will kinda self resolve anyway, since they are based on the whole concept of "checked" vs "unchecked". I think.

Otherwise, things look pretty good. Besides that, if we could possibly come up with a more descriptive name than feature167CheckEnabled for this, and I think I'll be happy :-). Maybe "extendedModificationWarnings" or something like that? Pretty much anything that would make it so I don't have to look at the github page to know what it is when I see it a year from now ;-).

Anyway, great work and thanks !

[jlsantiago0]

Thank you for evaluating my PR. I will change the name of the |feature167CheckEnabled | ||variable and add it to the PR. The never condition 'warn disabled' is handled exactly as before. warnAlways is the original mode when warn is enabled. if warnAlways is not enabled, but warn is enabled, then warnWhenLocalMods and warnWhenUserInteracts fine tune the when you want to be warned. You can enable or disable them independently. If all 3 are off, then it works like warn was disabled. You are right that it would be good if we could control these as a group in the UI. For instance when warn is disabled, it doesnt matter what the other 3 settings are. So they could be greyed out in the UI. I didnt know how to do that. Also if warn is enabled and warnAlways is enabled, then the warnWhenLocalMods and warnWhenUserInteracts could be greyed out as well. I dont know enough about how the menu items in the UI work to make that happen. ||

…

On 3/21/22 16:43, Evan Teran wrote: @jlsantiago0 <https://github.com/jlsantiago0> OK, I took a quick look and it generally looks great, you even did the UI parts :-) ! I have one question though. Is the intention for |warnAlways warnWhenLocalMods warnWhenUserInteracts | and the implied |warnNever| to be mututally exclusive, as in, it is only meaningful to pick one? If so, we can probably use a group to make the UI elements act as a radio select. I think the independent settings will kinda self resolve anyway, since they are based on the whole concept of "checked" vs "unchecked". I *think*. Otherwise, things look pretty good. Besides that, if we could possibly come up with a more descriptive name than |feature167CheckEnabled| for this, and I think I'll be happy :-). Maybe "extendedModificationWarnings" or something like that? Pretty much anything that would make it so I don't have to look at the github page to know what it is when I see it a year from now ;-). Anyway, great work and thanks ! — Reply to this email directly, view it on GitHub <#167 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMAKMDL7O52CLSLEX5Y3HDVBDUOPANCNFSM4QFD6Q4Q>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[eteran]

OK, this is a great jumping off point. I'll play with it to see what the best UI tweaks might be 👍🏻

[eteran]

@jlsantiago0 Hey just wanted to check in. No worries if you're busy, I'm in no rush; I just wanted to make sure you weren't waiting on me for more input or for me to make a tweak to the PR on my end.

Thanks!