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 warnings #19

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Fix warnings #19

wants to merge 17 commits into from

Conversation

NathanBnm
Copy link

Hi! I figured out that there were some warnings when building the app. Some things were deprecated so I fixed them.

Here were the warning messages:

../src/Wallpaperize.vala:27.31-27.56: warning: Gdk.Screen.get_primary_monitor has been deprecated since 3.22
../src/Wallpaperize.vala:29.9-29.35: warning: Gdk.Screen.get_monitor_geometry has been deprecated since 3.22
../src/Wallpaperize.vala:31.29-31.59: warning: Gdk.Screen.get_monitor_scale_factor has been deprecated since 3.22
../src/Application.vala:31.9-31.17: warning: Granite.Application.app_years has been deprecated since 0.4.2. Use 
../src/Application.vala:36.9-36.16: warning: Granite.Application.app_icon has been deprecated since 0.4.2. Use 
../src/Application.vala:37.9-37.16: warning: Granite.Application.main_url has been deprecated since 0.4.2. Use 
../src/Application.vala:38.9-38.15: warning: Granite.Application.bug_url has been deprecated since 0.4.2. Use 
../src/Application.vala:39.9-39.16: warning: Granite.Application.help_url has been deprecated since 0.4.2. Use 
../src/Application.vala:40.9-40.21: warning: Granite.Application.translate_url has been deprecated since 0.4.2. Use 
../src/Application.vala:41.9-41.21: warning: Granite.Application.about_authors has been deprecated since 0.4.2. Use 
../src/Application.vala:42.9-42.25: warning: Granite.Application.about_translators has been deprecated since 0.4.2. Use 
../src/Application.vala:44.9-44.26: warning: Granite.Application.about_license_type has been deprecated since 0.4.2. Use 
Compilation succeeded - 12 warning(s)

Please only merge this PR after merging my last one about translations otherwise you may have some conflicts.

This PR is ready for review

@NathanBnm
Copy link
Author

NathanBnm commented Apr 2, 2019

EDIT : I didn't expect that my previous commits from my other branch will be there too. As you want you can merge this to apply both this PR and #18 and close #18. Or you can still merge #18 and then merge this PR too. Sorry for duplicate.

@ryonakano
Copy link
Contributor

ryonakano commented Apr 9, 2019

@NathanBnm It seems you checked out your fix-warnings branch from your add-translations branch, not from master branch. This makes this PR looks like as if it has many changed files. I think you should change the base branch in order to keep this PR simple with the following command:

git rebase --onto master add-translations fix-warnings
git push origin fix-warnings

@ryonakano
Copy link
Contributor

Also, because the class Granite.Application is deprecated (see elementary/granite@2b89817), we may want to change it to Gtk.Application. Also, because Gtk.Application does not have fields like program_name or app_launcher, you can remove them. See the diff below to see what will change:

-public class Wallpaperize.Application : Granite.Application {
-    public const string PROGRAM_ID = "com.github.philip-scott.wallpaperize";
+public class Wallpaperize.Application : Gtk.Application {
     public const string PROGRAM_NAME = "Wallpaperize";
 
     construct {
         flags |= ApplicationFlags.HANDLES_OPEN;
-
-        application_id = PROGRAM_ID;
-        program_name = PROGRAM_NAME;
-        exec_name = PROGRAM_ID;
-        app_launcher = PROGRAM_ID;
-        build_version = "1.0";
+        application_id = "com.github.philip-scott.wallpaperize";
     }

(I also removed the constant PROGRAM_ID because it is never used more than twice once we remove those fields)

@NathanBnm
Copy link
Author

@NathanBnm It seems you checked out your fix-warnings branch from your add-translations branch, not from master branch. This makes this PR looks like as if it has many changed files. I think you should change the base branch in order to keep this PR simple with the following command:

git rebase --onto master add-translations fix-warnings
git push origin fix-warnings

Oh yes you're right, I didn't noticed that, thanks for reporting. I'm going to check that !

@NathanBnm
Copy link
Author

@ryonakano I was unable to rebase branch, I don't really now why. But I just reverted changes from the other branch and this is better now. I added your diff too 😄

I'm still learning, mistakes can happen 👎

src/Application.vala Outdated Show resolved Hide resolved
@NathanBnm
Copy link
Author

@Philip-Scott any news?

@NathanBnm
Copy link
Author

@Philip-Scott would tout please mind to review and merge this?

Copy link
Owner

@Philip-Scott Philip-Scott left a comment

Choose a reason for hiding this comment

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

The code looks good, I'd like to test it before merging but It should work correctly. Sorry, I turned off all Github notifications for this project a while back, and honestly just getting that motivation/energy to do coding related side-projects been hard lately.

@NathanBnm
Copy link
Author

No worries it would be awesome if you could take the time to review the PRs for this project 😁

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