Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let the user disable main window stealing focus when searching. #387

Merged
merged 3 commits into from
Mar 17, 2023
Merged

Let the user disable main window stealing focus when searching. #387

merged 3 commits into from
Mar 17, 2023

Conversation

tatsumoto-ren
Copy link
Contributor

When a search is triggered, the main window focuses itself even if it's already visible, which may be undesirable for some users. Let the user disable this behavior.

screenshot

mainwindow.cc Outdated
Comment on lines 2937 to 2944
if ( !isActiveWindow() )
{
qApp->setActiveWindow( this );
raise();
activateWindow();
if (cfg.preferences.raiseMainWindow) {
raise();
activateWindow();
}
shown = true;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a search action is triggered, does the mainwindow can be inactive at the same time?

Copy link
Contributor Author

@tatsumoto-ren tatsumoto-ren Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be visible but inactive, which I assume is what the isActiveWindow() call for.

Copy link
Owner

@xiaoyifang xiaoyifang Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there may be case when gd windows has invisible .at this case ,raise() should be called but without activateWindow()

does raise() need to put outside of the if condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this case ,raise() should be called but without activateWindow()

I'm going to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, apparently you have to disable raise() as well to prevent the main window from focusing itself.

Copy link
Owner

@xiaoyifang xiaoyifang Mar 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does popup dialog need to change accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does popup dialog need to change accordingly?

I think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the window is actually invisible, it is fine to raise it.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 14, 2023

please add some more informations about this PR
What's the consenquence without this PR and what's the benefit?

@tatsumoto-ren
Copy link
Contributor Author

please add some more informations about this PR
What's the consequence without this PR and what's the benefit?

For example, when I watch a video in MPV with Mpvacious, I have it automatically send the subtitles to GoldenDict. Every time a new subtitle appears on the screen, GoldenDict focuses itself, which is not always desirable. I only want to focus it myself when I need to. Without this PR I can only do it by having GoldenDict and MPV open in separate workspaces.

A screencast: https://invidious.baczek.me/watch?v=bvl-dtB0wLs

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 15, 2023

this should also be changed. when scanpopup is pinned and set at the top
image

https://github.com/xiaoyifang/goldendict/blob/31c72dacdc5943ccbd4078578f4663d332c8dc52/scanpopup.cc#L583-L597

@tatsumoto-ren
Copy link
Contributor Author

With the last commit popups work as intended.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 16, 2023

I think raiseWindowOnSearch should be called unraiseWindowOnSearch(or some other variable like this, the description of label should also be changed accordingly.

The reason is :
users mainwindow will be focused after search ,this should be the default behaviour.

after upgrade, users mainwindow will lost focus(as the checked box has been unchecked(I think) in the configuration file.
it will cause confusion to the current users.

@@ -580,7 +580,7 @@ void ScanPopup::engagePopup( bool forcePopup, bool giveFocus )
//QApplication::processEvents(); // Make window appear immediately no matter what
}
else
if ( ui.pinButton->isChecked() )
if ( ui.pinButton->isChecked() && cfg.preferences.raiseMainWindow)
{
// Pinned-down window isn't always on top, so we need to raise it
show();
Copy link
Owner

@xiaoyifang xiaoyifang Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show() should this method be called at all conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

show() should this method be called at all conditions?

I think so. I will change that.

@tatsumoto-ren
Copy link
Contributor Author

tatsumoto-ren commented Mar 16, 2023

I think raiseWindowOnSearch should be called unraiseWindowOnSearch(or some other variable like this, the description of label should also be changed accordingly.
The reason is :
users mainwindow will be focused after search ,this should be the default behaviour.
after upgrade, users mainwindow will lost focus(as the checked box has been unchecked(I think) in the configuration file.
it will cause confusion to the current users.

I think if the setting is on by default, which is the current behavior, then after updating the app the users won't notice any change unless they uncheck the setting. Correct me if I'm wrong.

We could call the setting dontRaiseWindowOnSearch, but then all the if blocks will have to be negated, which doesn't look pretty in the source code.

@xiaoyifang
Copy link
Owner

I think if the setting is on by default, which is the current behavior, then after updating the app the users won't notice any change unless they uncheck the setting. Correct me if I'm wrong.

ok,Let's use your way

last thing:
the changed codes need to follow the .clang-format guideline .
check this https://github.com/xiaoyifang/goldendict/blob/staged/howto/how%20to%20use%20.clang-format%20to%20format%20the%20code.md

@tatsumoto-ren
Copy link
Contributor Author

If I run clang-format on the 6 files I've changed, the diff is going to be huge, but I'll try.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 17, 2023

If I run clang-format on the 6 files I've changed, the diff is going to be huge, but I'll try.

only on the changed lines.

after set the clang format ,you can select the changes lines and use menu Edit->'Advanced'->format the selected lines

If you have difficult doing this ,Let's just pass the step , your current code format is also good enough.

@tatsumoto-ren
Copy link
Contributor Author

Alright.

@xiaoyifang
Copy link
Owner

xiaoyifang commented Mar 17, 2023

7f9edee this commit should be revert or reset .

too much unrelated line format changes will make future bug tracking difficult.

config.cc Outdated
Comment on lines 225 to 277
newTabsOpenAfterCurrentOne( false ),
newTabsOpenInBackground( true ),
hideSingleTab( false ),
mruTabOrder( false ),
hideMenubar( false ),
enableTrayIcon( true ),
startToTray( false ),
closeToTray( true ),
autoStart( false ),
doubleClickTranslates( true ),
selectWordBySingleClick( false ),
autoScrollToTargetArticle( true ),
escKeyHidesMainWindow( false ),
darkMode( false ),
darkReaderMode( false ),
alwaysOnTop( false ),
searchInDock( false ),

enableMainWindowHotkey( true ),
mainWindowHotkey( QKeySequence( "Ctrl+F11,F11" ) ),
enableClipboardHotkey( true ),
clipboardHotkey( QKeySequence( "Ctrl+C,C" ) ),

startWithScanPopupOn( false ),
enableScanPopupModifiers( false ),
scanPopupModifiers( 0 ),
ignoreOwnClipboardChanges( false ),
scanToMainWindow( false ),
ignoreDiacritics( false ),
ignorePunctuation( false ),
#ifdef HAVE_X11
// Enable both Clipboard and Selection by default so that X users can enjoy full
// power and disable optionally.
trackClipboardScan ( true ),
trackSelectionScan ( true ),
showScanFlag( false ),
// Enable both Clipboard and Selection by default so that X users can enjoy full
// power and disable optionally.
trackClipboardScan( true ),
trackSelectionScan( true ),
showScanFlag( false ),
#endif
pronounceOnLoadMain( false ),
pronounceOnLoadPopup( false ),
useInternalPlayer( InternalPlayerBackend::anyAvailable() ),
internalPlayerBackend( InternalPlayerBackend::defaultBackend() ),
checkForNewReleases( true ),
disallowContentFromOtherSites( false ),
enableWebPlugins( false ),
hideGoldenDictHeader( false ),
maxNetworkCacheSize( 50 ),
clearNetworkCacheOnExit( true ),
zoomFactor( 1 ),
helpZoomFactor( 1 ),
wordsZoomLevel( 0 ),
maxStringsInHistory( 500 ),
storeHistory( 1 ),
alwaysExpandOptionalParts( false )
, historyStoreInterval( 0 )
, favoritesStoreInterval( 0 )
, confirmFavoritesDeletion( true )
, collapseBigArticles( false )
, articleSizeLimit( 2000 )
, limitInputPhraseLength( false )
, inputPhraseLengthLimit( 1000 )
, maxDictionaryRefsInContextMenu ( 20 )
, synonymSearchEnabled( true ),
stripClipboard( false ),
raiseWindowOnSearch(true)
pronounceOnLoadMain( false ),
pronounceOnLoadPopup( false ),
useInternalPlayer( InternalPlayerBackend::anyAvailable() ),
internalPlayerBackend( InternalPlayerBackend::defaultBackend() ),
checkForNewReleases( true ),
disallowContentFromOtherSites( false ),
enableWebPlugins( false ),
hideGoldenDictHeader( false ),
maxNetworkCacheSize( 50 ),
clearNetworkCacheOnExit( true ),
zoomFactor( 1 ),
helpZoomFactor( 1 ),
wordsZoomLevel( 0 ),
maxStringsInHistory( 500 ),
storeHistory( 1 ),
alwaysExpandOptionalParts( false ),
Copy link
Owner

@xiaoyifang xiaoyifang Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not change these lines, seems the .clang-format settings has improper configuration .I'll check this file.

the original code is usually indent two spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

@xiaoyifang
Copy link
Owner

76fa9a8
further code format should be ok now.

@xiaoyifang xiaoyifang merged commit fb6d4f9 into xiaoyifang:staged Mar 17, 2023
@tatsumoto-ren tatsumoto-ren deleted the raise-checkbox branch March 17, 2023 01:58
@sonarcloud
Copy link

sonarcloud bot commented Mar 17, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants