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.

Sign up for GitHub

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

Jump to bottom

fondo, notes-up: init #56172

Merged
merged 3 commits into from Feb 23, 2019
Merged

fondo, notes-up: init #56172

merged 3 commits into from Feb 23, 2019

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

@davidak wanted notes-up.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace worldofpeace added the 6.topic: pantheon The Pantheon desktop environment label Feb 22, 2019
davidak
davidak approved these changes Feb 22, 2019
View reviewed changes
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

Looks good.

Thank you very much!

@dtzWill
Copy link
Member

dtzWill commented Feb 22, 2019

FWIW when trying to run this, it seems to bail (I'm not using full pantheon bits):

$ com.github.philip-scott.notes-up                                                                     12:12:34 on 19-02-22
[INFO 12:12:36.547813] Application.vala:155: Notes-Up version: 1.3
[INFO 12:12:36.547849] Application.vala:157: Kernel version: 4.20.11
[FATAL 12:12:36.692385] file /build/source/src/Widgets/Headerbar.vala: line 126: uncaught error: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.elementary.Contractor was not provided by any .service files (g-dbus-error-quark, 2)
[FATAL 12:12:36.723962] [Gtk] gtk_widget_get_parent: assertion 'GTK_IS_WIDGET (widget)' failed
[FATAL 12:12:36.724005] [Gtk] gtk_widget_freeze_child_notify: assertion 'GTK_IS_WIDGET (widget)' failed
zsh: segmentation fault (core dumped)  com.github.philip-scott.notes-up

Is this something to be resolved at the packaging level, or more intrinsic fault regarding compatibility in other environments?

@worldofpeace
Copy link
Contributor Author

I'm pretty sure fondo would only ever be useful in pantheon because of their greeter setup, etc.

But I think for notes-up that there's a special build flag that disables contractor support, because outside pantheon you won't have that interface.

So i probably should add a subpackage maybe that has -Dnoele=1 in cmakeFlags.

@dtzWill
Copy link
Member

dtzWill commented Feb 22, 2019

So i probably should add a subpackage maybe that has -Dnoele=1 in cmakeFlags.

As you see fit, and thank you. I mostly wanted to be sure it didn't indicate a dep that needed to be propagated or otherwise was a packaging bug.

Sounds like it's good as -is :).

@dtzWill
Copy link
Member

dtzWill commented Feb 22, 2019

Sorry for hijacking this a bit, but just FYI for anyone with this sort of issue:

Simply adding pantheon.contractor.enable = true; seems to have done the trick, no more crashing and doesn't seem to be intrusive or require full buy-in. But I wouldn't expect too much re:functionality since it likely is meant for other components too. Anyway, thanks!

@worldofpeace
Copy link
Contributor Author

Sorry for hijacking this a bit, but just FYI for anyone with this sort of issue:

Simply adding pantheon.contractor.enable = true; seems to have done the trick, no more crashing and doesn't seem to be intrusive or require full buy-in. But I wouldn't expect too much re:functionality since it likely is meant for other components too. Anyway, thanks!

No hijack done 😄 this should work wherever by default 366945a

@dtzWill
Copy link
Member

dtzWill commented Feb 22, 2019

❤️ thanks! Yes this looks great!

@worldofpeace
Copy link
Contributor Author

@GrahamcOfBorg eval

@worldofpeace
Copy link
Contributor Author

ping @jtojnar

@jtojnar
Copy link
Contributor

jtojnar commented Feb 22, 2019

Looks about right.

@worldofpeace
notes-up: default without pantheon support
ac2546b 
This adds pantheon.notes-up which will only work in pantheon.
@worldofpeace worldofpeace merged commit 904732d into NixOS:master Feb 23, 2019
@worldofpeace worldofpeace deleted the elementary-post branch February 23, 2019 00:17
@worldofpeace
Copy link
Contributor Author

Thanks for the reviews everyone 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@davidak davidak davidak approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@worldofpeace @dtzWill @jtojnar @davidak @GrahamcOfBorg