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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

opt: add translation windows when invoked from console #1173

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

xiaoyifang
Copy link
Owner

@xiaoyifang xiaoyifang commented Sep 27, 2023

show the translation in popup

goldendict -s [word]

show the translation in main windows

goldendict -m [word]

Without the above argument, the logic will follow the logic:
if 馃挕 checked, use popup ,or use mainwindow.

@xiaoyifang xiaoyifang changed the title test opt: add translation windows when invoked from console Sep 27, 2023
@xiaoyifang xiaoyifang force-pushed the opt/commandline-popup branch 3 times, most recently from edb7655 to bc99329 Compare September 27, 2023 00:23
@shenlebantongying
Copy link
Collaborator

shenlebantongying commented Sep 27, 2023

From the help message, it is almost impossible to deduce that the thing followed by -w must be popup or main. I think this might be confusing because a flag following a text choice is unconventional. This may need an example but that will make the help message too long.


Why not add 2 new flags

  • -p --scanpopup Force the word to be translated in scanpopup
  • -m --main-window Force the word to be translated in the mainwindow

If no flags exist, then it will be decided by the 馃挕 on toolbar.

There should be something that can translate words directly

if ( message.left( 15 ) == "translateWord: " ) {
if ( scanPopup )
if ( ( consoleWindowOnce == "popup" ) && scanPopup )
Copy link
Collaborator

Choose a reason for hiding this comment

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

The && is questionable.

Right now, without any flags, the 馃挕 decided where the word will go when just using goldendict <word>.

After this, turning on the 馃挕 will always go to main window with goldendict <word>.

I think the -w should be an "override" or "enforce" flag that says "This must be done" instead of requiring it to send to scanpopup.


Copy link
Owner Author

@xiaoyifang xiaoyifang Sep 27, 2023

Choose a reason for hiding this comment

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

if ( scanPopup )

will always be true.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think the -w should be an "override" or "enforce" flag that says "This must be done" instead of requiring it to send to scanpopup.

agree

@xiaoyifang
Copy link
Owner Author

If no flags exist, then it will be decided by the 馃挕 on toolbar.

seems this logic has been lost.

@shenlebantongying
Copy link
Collaborator

seems this logic has been lost.

Right 馃槄.

Should have replaced most if ( scanPopup ) with if( enableScanningAction->isChecked() ).

@xiaoyifang
Copy link
Owner Author

  • -p --scanpopup Force the word to be translated in scanpopup

p is already been used

@xiaoyifang xiaoyifang force-pushed the opt/commandline-popup branch 2 times, most recently from 48e3a86 to ac6b3a7 Compare September 27, 2023 05:41
@shenlebantongying
Copy link
Collaborator

p is already been used

then how about -s --scanpopup

goldendict -s <word>

is much shorter than

goldendict -w popup <word>

@sonarcloud
Copy link

sonarcloud bot commented Sep 28, 2023

SonarCloud Quality Gate failed.聽 聽 Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@xiaoyifang
Copy link
Owner Author

p is already been used

then how about -s --scanpopup

goldendict -s <word>

is much shorter than

goldendict -w popup <word>

done

@xiaoyifang xiaoyifang merged commit ef6e878 into staged Sep 30, 2023
12 of 13 checks passed
@xiaoyifang xiaoyifang deleted the opt/commandline-popup branch September 30, 2023 03:40
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.

None yet

2 participants