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

gs-watcher-x11: Migrate from dbus-glib to gdbus #230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yetist
Copy link
Member

@yetist yetist commented Aug 14, 2020

No description provided.

@sc0w
Copy link
Member

sc0w commented Aug 14, 2020

the indentations with new style inside old style makes the code unreadable at some points

why not just respect old style?

@yetist
Copy link
Member Author

yetist commented Aug 14, 2020

Someone once told me that the unmodified code should be kept, otherwise it will increase the workload of the review.

To be honest, the coding style often causes me trouble when commit code.

@yetist yetist requested a review from a team August 14, 2020 12:52
@raveit65
Copy link
Member

raveit65 commented Aug 14, 2020

Same as in mate-desktop/mate-control-center#579
Why not fixing code-style problems in older PRs first.

Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

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

please respect old style to avoid missaligned indentations

@yetist
Copy link
Member Author

yetist commented Aug 14, 2020

I really don't know what to do?

  1. Respect the old style and still use Tab indentation.
  2. Use the new style and indent with 4 spaces, which is what this PR does.
  3. Submit 2 times in the PR, the first time only convert the code style to the new format, and the second time modify the code.

@yetist
Copy link
Member Author

yetist commented Aug 14, 2020

The current code respects the old style and uses Tab for indentation. I think it may pass the review.

@yetist yetist requested a review from sc0w August 14, 2020 13:29
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
@sc0w
Copy link
Member

sc0w commented Aug 14, 2020

new warnings in the logs:

gs-watcher-x11.c:408:38: warning: Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption
                        g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(u)"))) {
                                                          ^~~~~~~~~~~~~~~~~~~~~~

gs-watcher-x11.c:414:38: warning: Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption
                        g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(s)"))) {
                                                          ^~~~~~~~~~~~~~~~~~~~~~

@raveit65
Copy link
Member

src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
src/gs-watcher-x11.c Outdated Show resolved Hide resolved
@sc0w
Copy link
Member

sc0w commented Aug 15, 2020

I still see the warnings:

gs-watcher-x11.c:408:40: warning: Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption
            g_variant_is_of_type (parameters, G_VARIANT_TYPE("(u)")))
                                              ^~~~~~~~~~~~~~~~~~~~~

gs-watcher-x11.c:415:40: warning: Casting a non-structure type to a structure type and accessing a field can lead to memory access errors or data corruption
            g_variant_is_of_type (parameters, G_VARIANT_TYPE("(s)")))
                                              ^~~~~~~~~~~~~~~~~~~~~

@yetist
Copy link
Member Author

yetist commented Aug 16, 2020

I still see the warnings:

Yes, I noticed it, but I don't know how to fix it, it seems that there is no problem with that.
https://github.com/search?q=org%3AGNOME+g_variant_is_of_type&type=Code

@sc0w sc0w self-requested a review August 16, 2020 11:45
Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

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

ok, I think we can live with that

@raveit65
Copy link
Member

@yetist
Can you please squash all commits in one?
Than i can start to test it.

@yetist
Copy link
Member Author

yetist commented Aug 16, 2020

@raveit65

OK, done.

@raveit65
Copy link
Member

offtopic on
I am using this commit as patch to test it with a RPM package.
When opening the with git format-patch.... generated patch in pluma the tab size wasn't shown correct and i thought there was an indent issue.
After resetting the tab size from 8 --> 7 --> 8 px the patch was displayed correct in pluma.
I did run 2 times in this issue the last days, so it is reproducible for me.
Maybe someone other see this issue with pluma too?
offtopic off

@raveit65
Copy link
Member

@yetist
Is this really tested?

@raveit65
Copy link
Member

Try killall mate-screensaver && mate-screensaver --debug

@raveit65
Copy link
Member

@yetist
Screensaver doesn't start and there is a run time warning.

@yetist
Copy link
Member Author

yetist commented Aug 18, 2020

sorry, please keep this PR open, I will migrate mate-session-manager soon.

@fossfreedom
Copy link

Hi - just a quick note since I am looking at budgie-screensaver which has the same issues with the deprecated dbus-glib-1 library.

Think this PR should be marked as draft BTW.

https://github.com/mate-desktop/mate-screensaver/blob/master/configure.ac needs reworking to remove the reliance on the library. Doing this also reveals that https://github.com/mate-desktop/mate-screensaver/blob/master/src/gs-listener-dbus.c will also need a rewrite as well.

@lukefromdc
Copy link
Member

Any updates on this?

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.

5 participants