Skip to content

Conversation

@Mte90
Copy link
Contributor

@Mte90 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.

Copy link
Member

@mitya57 mitya57 left a comment

Choose a reason for hiding this comment

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

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!

sys.exit(1)

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

Choose a reason for hiding this comment

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

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?

@Mte90
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
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
Copy link
Contributor Author

Mte90 commented Oct 12, 2019

No problem @mitya57 :-)

@mitya57 mitya57 merged commit ec3e50f into retext-project:master Oct 14, 2019
@mitya57
Copy link
Member

mitya57 commented Oct 14, 2019

Merged now, thank you!

parser.addVersionOption()
previewOption = QCommandLineOption('preview',
QApplication.translate('main', 'Open the files in preview mode'))
newWindowOption = QCommandLineOption('new-window',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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