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

Fix #148 - Open documents into new tab rather than new window #476

Merged
merged 1 commit into from Oct 14, 2019

Conversation

@Mte90
Copy link
Contributor

Mte90 commented Oct 6, 2019

In this way if there is already ReText instance, the second one send the file to the other and kill himself.

ReText/window.py Show resolved Hide resolved
Copy link
Member

mitya57 left a comment

Some general comments:

  • The window is briefly shown and then closed. I think the new code should be moved before window.show() line.

  • We also support Windows, macOS and other platforms where D-Bus is not available. ReText should continue working on those platforms.

  • I think some people would like to still be able to create multiple windows. Maybe add a command-line option to start a new window? (And ideally expose it in the .desktop file).

Otherwise thanks for your pull request!

ReText/__main__.py Outdated Show resolved Hide resolved
ReText/__main__.py Outdated Show resolved Hide resolved
sys.exit(1)

connection = QDBusConnection.sessionBus()
connection.registerObject('/', window, QDBusConnection.ExportAllSlots)

This comment has been minimized.

Copy link
@mitya57

mitya57 Oct 9, 2019

Member

This also exports some slots on org.qtproject.Qt.QWidget interface that should not be exposed publicly. Like setWindowModified and setWindowTitle. Is there any way to avoid that?

ReText/__main__.py Outdated Show resolved Hide resolved
@Mte90

This comment has been minimized.

Copy link
Contributor Author

Mte90 commented Oct 10, 2019

Right now is missing:

  • I don't know how to add a new parameter to ReText, do you have any suggestion where is the code? I see that manually you are checking the sys.argv, but for new parameters is not a scalable way
  • To expose only the method of ReTextWindow that are not part of QWindow we need to create a new QDbusAdapter with the list of all the methods. Maybe we can create a wrapper class that include only the ones that we really want.
@mitya57

This comment has been minimized.

Copy link
Member

mitya57 commented Oct 12, 2019

Thanks for your work! I am on a vacation now, will finish the implementation myself next week.

@Mte90

This comment has been minimized.

Copy link
Contributor Author

Mte90 commented Oct 12, 2019

No problem @mitya57 :-)

Fixes #148.
@mitya57 mitya57 force-pushed the Mte90:148 branch from 8bc8309 to ec3e50f Oct 14, 2019
@mitya57 mitya57 merged commit ec3e50f into retext-project:master Oct 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mitya57

This comment has been minimized.

Copy link
Member

mitya57 commented Oct 14, 2019

Merged now, thank you!

@@ -74,7 +75,10 @@ def main():
parser.addVersionOption()
previewOption = QCommandLineOption('preview',
QApplication.translate('main', 'Open the files in preview mode'))
newWindowOption = QCommandLineOption('new-window',

This comment has been minimized.

Copy link
@Mte90

Mte90 Oct 14, 2019

Author Contributor

I didn't know of this class in Qt, my fault

This comment has been minimized.

Copy link
@mitya57

mitya57 Oct 14, 2019

Member

There is also Python's argparse module, but Qt's is better, as it supports passing some standard flags (like -style or -platform) to Qt.

ReText/__main__.py Show resolved Hide resolved
ReText/__main__.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.