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

Rework Application Class #702

Merged
merged 13 commits into from
Feb 28, 2023
Merged

Rework Application Class #702

merged 13 commits into from
Feb 28, 2023

Conversation

Marukesu
Copy link
Contributor

Modernize the Application class to better reflect the GLib.Application API, it now have a distinction between local and primary, command line and DBus invocations.

The command line logic was split and simplified:

  • the working directory of the local invocation is used instead of the primary when creating a new tab/window.
  • --working-directory is a command line only option, with DBus invocations expecting the cwd option in the platform-data to be used instead.
  • --commandline works without =, matching the behaviour of others long options.
  • ApplicationFlags.CAN_OVERRIDE_APP_ID was added to facilitate testing changes without needing to close a already open instance of terminal.

Test are added, to assure that we don't cause any regression in the command line/options parsing when modifying the class in the future.

* move the ENTRIES constant to top
* creation method before construct
* instance - static, protected - public - private, ordering

code style was updated and some methods simplified.
Gtk.Application has API to list and get windows registered with the
application. use that instead of keeping track of windows by ourselves
local_command_line() and handle_local_options() are responsible for the
command line parsing and adjusting the options for usage in command_line()
that handles the actual program logic in the primary instance.

this simplifies the primary instance work, and keep command line specific
behavior to be parsed only from command line invocation.
tries to guarantee the behavior of the command line interface, both local
and primary
@Marukesu Marukesu requested a review from a team February 12, 2023 19:41
@jeremypw
Copy link
Collaborator

@Marukesu I do not pretend to understand every aspect of this PR but it looks good and the test framework is impressive. However, when I install and run it from a different terminal app (I've tried XTerm and Blackbox) using no parameters, I get an immediate crash. The backtrace is :

Thread 1 "io.elementary.t" received signal SIGSEGV, Segmentation fault.
0x0000555555571c17 in __lambda5_ (self=0x5555555e21c0, id=0x555555eb8e00 "0", process=0x5555555f4620 "gdb io.elementary.terminal", exit_status=0) at ../src/Application.vala:120
120	                    if (terminal.terminal_id == id) {
(gdb) bt
#0  0x0000555555571c17 in __lambda5_
    (self=0x5555555e21c0, id=0x555555eb8e00 "0", process=0x5555555f4620 "gdb io.elementary.terminal", exit_status=0) at ../src/Application.vala:120
#1  0x0000555555572122 in ___lambda5__terminal_dbus_finished_processPython Exception <class 'ValueError'>: Variable 'static_fundamental_type_nodes' not found.

    (_sender=, terminal_id=0x555555eb8e00 "0", process=0x5555555f4620 "gdb io.elementary.terminal", exit_status=0, self=0x5555555e21c0) at ../src/Application.vala:111
#2  0x0000555555573d2e in g_cclosure_user_marshal_VOID__STRING_STRING_INT
    (closure=0x555555601110, return_value=0x0, n_param_values=4, param_values=0x7fffffffd5d0, invocation_hint=0x7fffffffd550, marshal_data=0x0) at ../src/DBus.vala:22
#3  0x00007ffff7e17d2f in g_closure_invoke () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
#4  0x00007ffff7e33c36 in  () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
#5  0x00007ffff7e35614 in g_signal_emit_valist () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
#6  0x00007ffff7e35863 in g_signal_emit () at /lib/x86_64-linux-gnu/libgobject-2.0.so.0
#7  0x0000555555573c46 in terminal_dbus_process_finishedPython Exception <class 'ValueError'>: Variable 'static_fundamental_type_nodes' not found.

    (self=, terminal_id=0x555555ebc3f0 "0", process=0x555555dbbd50 "gdb io.elementary.terminal", exit_--Type <RET> for more, q to quit, c to continue without paging--
status=0, error=0x7fffffffd920) at ../src/DBus.vala:30
#8  0x0000555555574805 in _dbus_terminal_dbus_process_finishedPython Exception <class 'ValueError'>: Variable 'static_fundamental_type_nodes' not found.

    (self=, _parameters_=0x7fffdc010a60, invocation=0x555555ec4ec0)
    at /home/jeremyw/Documents/Elementary/terminal/build/DBus.c:537
#9  0x0000555555574997 in terminal_dbus_dbus_interface_method_call
    (connection=0x7fffe400b020, sender=0x7fffdc011bd0 ":1.136", object_path=0x7fffdc011f60 "/io/elementary/terminal", interface_name=0x7fffdc011f10 "io.elementary.terminal", method_name=0x7fffdc006790 "ProcessFinished", parameters=0x7fffdc010a60, invocation=0x555555ec4ec0, user_data=0x5555555fdc80)
    at /home/jeremyw/Documents/Elementary/terminal/build/DBus.c:570
#10 0x00007ffff7d34bfe in  () at /lib/x86_64-linux-gnu/libgio-2.0.so.0
#11 0x00007ffff7eb8c44 in g_main_context_dispatch () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#12 0x00007ffff7f0d6c8 in  () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#13 0x00007ffff7eb63e3 in g_main_context_iteration () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#14 0x00007ffff7d0bfb5 in g_application_run () at /lib/x86_64-linux-gnu/libgio-2.0.so.0
#15 0x0000555555573aab in _vala_main (args=0x7fffffffde28, args_length1=1)
--Type <RET> for more, q to quit, c to continue without paging--
    at ../src/Application.vala:227
#16 0x0000555555573af8 in main (argc=1, argv=0x7fffffffde28) at ../src/Application.vala:226
(gdb) 

@jeremypw
Copy link
Collaborator

Putting a null check in \src\Application.vala line 120 stops the crash at least.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 18, 2023

Other observations so far when launching from gnome-terminal after compiling in the null check to stop crash:

  • Using the -h works as expected and does not launch io.elementary.terminal.
  • Using the -v option in a different terminal app opens io.elementary.terminal as well as returning the version . This does not happen with master branch. Moreover, a new tab is opened at the current directory in addition to restoring saved tabs.
  • Using -n and -t works as master. See issue #498 which persists
  • Using -n and -t (when an instance of io.elementary.terminal already running) works as master
  • Using -w works as master. Also restores saves tabs
  • Using -w in combination with -n or -t works as master.

@jeremypw
Copy link
Collaborator

  • Using-e flag works the same as in master, additional arguments ignored unless preceded by further flags
  • Using --execute= flag works if there is no space after =. If there is a space then the app opens but no tab is created and no error message given. This differs from master, which gives a warning about spaces both in the launching terminal and in an additional tab in the launched terminal.

@jeremypw
Copy link
Collaborator

As far as tested so far the -x flag works the same as master. Further testing will be done later.

@Marukesu
Copy link
Contributor Author

@jeremypw the last commits should fix opening the terminal without arguments and the -v argument not returning. i would like to add test cases for them too, but i couldn't find a way to do that right now.

Using -n and -t works as master. See issue #498 which persists

about #498, it's out of scope for this PR. The issue is that the restoration code will create a tab in the home directory if no tab is to be restored. I'm working in cleanup this code and fix it in another branch.

Using --execute= flag works if there is no space after =. If there is a space then the app opens but no tab is created and no error message given. This differs from master, which gives a warning about spaces both in the launching terminal and in an additional tab in the launched terminal.

the new code ignores -e and -x if they are a empty line arguments. i don't really think crating a new tab with a warning is the best way to warn the user about it, we could either print a warning and return while in the local handling, or show a error dialog/notification if empty lines was sent to the primary instance.

@jeremypw
Copy link
Collaborator

@Marukesu

about #498, it's out of scope for this PR. The issue is that the restoration code will create a tab in the home directory if no tab is to be restored. I'm working in cleanup this code and fix it in another branch.

That's fine, I wasn't expecting it to be fixed in this PR.

the new code ignores -e and -x if they are a empty line arguments. i don't really think crating a new tab with a warning is the best way to warn the user about it, we could either print a warning and return while in the local handling, or show a error dialog/notification if empty lines was sent to the primary instance.

I agree that creating a new tab is not necessary. Either a printed warning or a notification depending on means of calling would be good.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 19, 2023

While io.elementary.terminal -w ~/Music works, io.elementary.terminal --working-directory=~/Music does not. However the same happens in master. io.elementary.terminal --working-directory=/home/<username>/Music does work.

@jeremypw
Copy link
Collaborator

jeremypw commented Feb 19, 2023

There is a slight difference between master and PR regarding the -x flag. Running io.elementary.terminal -x ls from a directory other the home directory results in a new tab containing a listing of the current directory with this PR but with master it results in a listing of the home directory. I think the behaviour of this PR is more correct.

@jeremypw jeremypw merged commit 7a60d50 into master Feb 28, 2023
@jeremypw jeremypw deleted the maru/gapplication branch February 28, 2023 13:15
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.

3 participants