Clock applet/wayland: position calendar window same as in x11

[lukefromdc]

Had to clean up indents, but could NOT find the cause of repeated travis build failures. Mate-panel with wayland enabled already requires gtk-layer-shell, I made similar revisions to the clock's makefile.am as in wncklet, but while my local build found the included symbols from gtk-layer-shell.h the travis builds did not.

What am I missing here?

[raveit65]

Not sure about, but it seems that clang complains about a compiler error. https://app.travis-ci.com/github/mate-desktop/mate-panel/jobs/604956193#L3881

clang: error: linker command failed with exit code 1 (use -v to see invocation)

make[5]: *** [Makefile:822: clock-applet] Error 1

In CI we are using clang to compile the packages. https://app.travis-ci.com/github/mate-desktop/mate-panel/jobs/604956193#L1467 compiler: /usr/share/clang/scan-build-14/bin/../libexec/ccc-analyzer I guess at home you're using gcc. For this reason you didn't run in this compiler error. Here the compiler use these options. https://app.travis-ci.com/github/mate-desktop/mate-panel/jobs/604956193#L1485

+ scan-build -enable-checker deadcode.DeadStores -enable-checker alpha.deadcode.UnreachableCode -enable-checker alpha.core.CastSize -enable-checker alpha.core.CastToStruct -enable-checker alpha.core.IdenticalExpr -enable-checker alpha.core.SizeofPtr -enable-checker alpha.security.ArrayBoundV2 -enable-checker alpha.security.MallocOverflow -enable-checker alpha.security.ReturnPtrRange -enable-checker alpha.unix.SimpleStream -enable-checker alpha.unix.cstring.BufferOverlap -enable-checker alpha.unix.cstring.NotNullTerminated -enable-checker alpha.unix.cstring.OutOfBounds -enable-checker alpha.core.FixedAddr -enable-checker security.insecureAPI.strcpy --keep-cc --use-cc=clang --use-c++=clang++ -o html-report make -j 2

But, i 've no idea to fix it. Sorry

[cwendling]

The reason is simple: clock is not built in-process (https://app.travis-ci.com/github/mate-desktop/mate-panel/jobs/604956193#L1472), yet the Wayland flags are only set in such a case (see diff), which kind of makes sense as out-of-process doesn't work there. So I guess you need an extra check somewhere only to enable wayland support for in-process applets.

[raveit65]

I guess we need --with-in-process-applets=all configure option in build.yml. https://github.com/mate-desktop/mate-panel/blob/master/.build.yml#L143

[cwendling]

@raveit65 yes, but I'd say in a new job, because we should also test how it work without (e.g. this PR would have broken the default setup)

[raveit65]

Ok, but how you will test the code of this PR if not build in-process. And yes, using x11 with in-process or out-of-process should be possible.

Offtopic on: Since i am using mate-media sound applet build in-process on my production daily used system, the panel crashed very often. That's the downside if all applets are build in-process. There are some open issues from media-applet which needs to be fixed before using the applet build in-process. Also, the sound part from mate-settings-daemon is involved. Currently i have the feeling my MATE desktop isn't stable anymore. Well, i will open a report when i have actual stack-traces. So i am thinking it might be a good option to have a configure option --with-in-process-applets for the other applet packages. offtopc off

[cwendling]

Ok, but how you will test the code of this PR if not build in-process. And yes, using x11 with in-process or out-of-process should be possible.

I mean having more test jobs, one doing the in-process, the other the out-of-process. And they should both succeed, if all is well

Offtopic on: Since i am using mate-media sound applet build in-process on my production daily used system, the panel crashed very often. That's the downside if all applets are build in-process. There are some open issues from media-applet which needs to be fixed before using the applet build in-process. Also, the sound part from mate-settings-daemon is involved. Currently i have the feeling my MATE desktop isn't stable anymore. Well, i will open a report when i have actual stack-traces. So i am thinking it might be a good option to have a configure option --with-in-process-applets for the other applet packages. offtopc off

Yeah that's the risk, more code is in the same process, more risk of bugs… and possibly the applets have more bug in-process because the environment is not exactly the same and they do things they shouldn't.

Another solution would be to create a whole new out-of-process mechanism, like a custom protocol (DBus I guess) for applets to expose what they need. The downside is that it's more complex, and either weird or a lot more restricted as to what an applet could do (I don't see how e.g. the system monitor applet would work but if it uploads a frame -- still doable, but a lot more tricky). But the advantage is obviously that less "random" (3rd party) code runs in the panel process, so less likely to mess things up. that's just a thought though, because it's complex and I'm not sure if it's worth it.

[lukefromdc]

More help on these would be a benefit, as my understanding of the build system is still quite limited. For this one we still support building in or out of process (the c code differences are minimal as can be seen in my mate-applets work), so I will try to put wayland enablement behind an in-process switch if that has already been done for wncket to give me a template for it.

[raveit65]

Before this PR it was possible to build all applets (including clock) in-process or out-of-process with this configure flag --with-in-process-applets=all/none. In my understanding you don't need change anything in clock applet code to build in process or out-of-process. The configure flag should do the job, ihmo.

[raveit65]

See https://github.com/mate-desktop/mate-panel/blob/master/applets/clock/Makefile.am#L58

if CLOCK_INPROCESS
.
.
.
.

[raveit65]

Hmm, now build fails in the make distcheck part in afterscripts. https://github.com/mate-desktop/mate-panel/blob/master/.build.yml#L164 Normal compile part works fine.

[raveit65]

Ok, i found the DISTCHECK_CONFIGURE_FLAGS https://github.com/mate-desktop/mate-panel/blob/master/Makefile.am#L14

DISTCHECK_CONFIGURE_FLAGS = \
    --enable-gtk-doc \
    --disable-introspection \
    --enable-compile-warnings=no \
    CFLAGS='-Wno-deprecated-declarations'

[lukefromdc]

I just completely split the x11 and wayland code paths, limiting the wayland part to in-process builds. Got local successful builds out of process, in-process with wayland disabled,. and in-process with wayland enabled

[lukefromdc]

I found out of process builds broken prior to that change, not sure when in this work I broke that but it's fixed now. The last problem was a closing bracket inside instead of outside the#endif statements to limit the wayland code to wayland/in-process builds

[lukefromdc]

At the moment, building a new out of process mechanism is beyond my capacity

[raveit65]

Found out that the build failure in make distcheck part exists also in master branch. In both branches (master and wayland-calendar) this command failed to run. ./autogen.sh --with-in-process-applets=all --enable-x11 --enable-wayland && make && make distcheck But this command runs fine. ./autogen.sh --with-in-process-applets=none --enable-x11 --enable-wayland && make && make distcheck

When adding --with-in-process-applets=all to distcheck

DISTCHECK_CONFIGURE_FLAGS = \
    --enable-gtk-doc \
    --disable-introspection \
    --enable-compile-warnings=no \
    --with-in-process-applets=all \
    CFLAGS='-Wno-deprecated-declarations'

both previous commands are running fine :) I think it is OK to add the --with-in-process-applets=all only to distcheck as the main usage of distcheck is to build our tarballs for releasing. And we see here in CI that the new code compiles fine with both options. Or fixing the distcheck error for both build cases, which seems complicate. What do you think?

[cwendling]

@raveit65 distcheck failing usually means files are missing in the generated tarball. So I don't think changing the flags is teh way to go, but we should find out which files are missing, and include them (either using the dist_ prefix, or adding them to EXTRA_DIST or alike)

[raveit65]

@cwendling https://app.travis-ci.com/github/mate-desktop/mate-panel/jobs/605019674#L4129 I agree, but org.mate.panel.applet.ClockAppletFactory.service.in is missing for distcheck (without in-process) only when compiling (with in-process) before distcheck was done. That means distcheck have to detect which option was used before during the normal make command. Honestly, i have no idea how should that work :) Also, i am in worried that we see the problem with the other applets too, when it is fixed for clock applet.

Anyway, i will check if a tarball generated with in-process does work for a out-of-process builds, to see if files are missing.

[raveit65]

Ok, you're right. My idea doesn't work. With in-process in distcheck and generating a tarball with ./autogen.sh --with-in-process-applets=all --enable-x11 --enable-wayland && make && make distcheck, building from the tarball with ./configure --with-in-process-applets=none --enable-x11 --enable-wayland && make fails :\

[cwendling]

Yeah I'm not surprised. I'll try and take a look in a bit

[cwendling]

@raveit65 see #1378

[raveit65]

@lukefromdc Rebasing from master should fix the distcheck issue when build in-process. The remaining question is what happens when building out-of-process with the new code. Edit: opps, already tested ./autogen.sh --with-in-process-applets=none --enable-x11 --enable-wayland && make && make distcheck works fine. So, --with-in-process-applets=all can be removed fom CI after running CI one time, ihmo,.......or not :)

[lukefromdc]

This work can take all the time it needs to get it right. My hope would be to have an basic wayland session that looks and feels like the x11 session available in MATE 1.28, and maybe have one distros can treat as a default desktop by 1.32 or so, similar to the GTK 3 transition except for never dropping x11 suppport.

I believe we need to be ready no matter what comes out of the x11 vs wayland contest over the years.

[raveit65]

This work can take all the time it needs to get it right. My hope would be to have an basic wayland session that looks and feels like the x11 session available in MATE 1.28, and maybe have one distros can treat as a default desktop by 1.32 or so, similar to the GTK 3 transition except for never dropping x11 suppport.

I believe we need to be ready no matter what comes out of the x11 vs wayland contest over the years.

When we have a session file for MATE-Layer-Shell which makes it possible to start the session from desktop-manager login, similar what we use for x11, i am happy to start with both sessions for 1.28 in fedora. Same what gnome did with fc15 in May 24, 2011 ;)

Edit: I was too optimistic, 2011 was gnome3 release. I meant f21 in September 25, 2014

[lukefromdc]

That is exactly my objective for this work: For MATE to have a desktop session in wayland just as GNOME and KDE do. I believe that will be necessary going forward-and it's looking like wayland supports lighter code and may itself be lighter as well though correct me if I am wrong about that. The kludges necessary for wayland (and there are many) still seem less numerous than the kludges needed in x11 where scaling and desktop resizing create so many hassles

[lukefromdc]

Does anyone else want to further review this or should I proceed to merge it?

[lukefromdc]

The problem with using 8 spaces is twofold: One is that some of this code is deeply nested and takes a lot of space on screen when 8 spaces are used. All the same, on closer examination all the rest of this particular file is 8 space indents. Will change this

[raveit65]

When i found time i will fix the tab-size dilemma in a new PR, or replace them with spaces. I did that already for some files in mate-panel or caja a while ago, but we have a lot of files with mixed tap-size. 2 or 3 different tab-sizes in a file isn't nice when you reading code. For that reason i prefer using only spaces. Anyway, using 8 spaces for a tab is too large.

[cwendling]

[…] All the same, on closer examination all the rest of this particular file is 8 space indents. Will change this

Discrepancies from 8-spaces I can see is this (ignoring your changes):

diff --git a/applets/clock/clock.c b/applets/clock/clock.c
index 4b203e1f..30fea391 100644
--- a/applets/clock/clock.c
+++ b/applets/clock/clock.c
@@ -317,16 +317,16 @@ clock_set_timeout (ClockData *cd,
                         timeouttime = ((999 - itime_ms % 1000) * 86.4) / 100 + 1;
                 }
         } else {
-                 struct timeval tv;
+                struct timeval tv;

                 gettimeofday (&tv, NULL);
-                 timeouttime = (G_USEC_PER_SEC - tv.tv_usec)/1000+20;
+                timeouttime = (G_USEC_PER_SEC - tv.tv_usec)/1000+20;

                 /* timeout of one minute if we don't care about the seconds */
-                 if (cd->format != CLOCK_FORMAT_UNIX &&
+                if (cd->format != CLOCK_FORMAT_UNIX &&
                     !cd->showseconds &&
                     (!cd->set_time_window || !gtk_widget_get_visible (cd->set_time_window)))
-                         timeouttime += 1000 * (59 - now % 60);
+                        timeouttime += 1000 * (59 - now % 60);
          }

         cd->timeout = g_timeout_add (timeouttime,
@@ -452,7 +452,7 @@ get_updated_timeformat (ClockData *cd)
         use_lctime = (env_language != NULL) && (env_lc_time != NULL) && (g_strcmp0 (env_language, env_lc_time) != 0);

         if (use_lctime) {
-            g_setenv("LANGUAGE", env_lc_time, TRUE);
+                g_setenv("LANGUAGE", env_lc_time, TRUE);
         }

         if (cd->format == CLOCK_FORMAT_12)
@@ -497,7 +497,7 @@ get_updated_timeformat (ClockData *cd)

         /* Set back LANGUAGE the way it was before */
         if (use_lctime) {
-            g_setenv("LANGUAGE", env_language, TRUE);
+                g_setenv("LANGUAGE", env_language, TRUE);
         }

         result = g_locale_from_utf8 (clock_format, -1, NULL, NULL, NULL);
@@ -707,7 +707,7 @@ update_tooltip (ClockData * cd)
          * This can prevent problems with OpenGL on some drivers */
         old_tip = gtk_widget_get_tooltip_text (cd->panel_button);
         if (g_strcmp0 (old_tip, tip))
-            gtk_widget_set_tooltip_text (cd->panel_button, tip);
+                gtk_widget_set_tooltip_text (cd->panel_button, tip);

         g_free (old_tip);
         if (!cd->showdate)
@@ -2518,9 +2518,9 @@ parse_gsettings_cities (ClockData *cd, gchar **values)
         context = g_markup_parse_context_new (&location_parser, 0, &data, NULL);

         if (values) {
-            for (i = 0; values[i]; i++) {
-                    g_markup_parse_context_parse (context, values[i], strlen(values[i]), NULL);
-            }
+                for (i = 0; values[i]; i++) {
+                        g_markup_parse_context_parse (context, values[i], strlen(values[i]), NULL);
+                }
         }

         g_markup_parse_context_free (context);
@@ -2957,13 +2957,13 @@ location_changed (GObject *object, GParamSpec *param, ClockData *cd)
 static void
 location_name_changed (GObject *object, ClockData *cd)
 {
-    location_update_ok_sensitivity (cd);
+        location_update_ok_sensitivity (cd);
 }

 static void
 location_timezone_changed (GObject *object, GParamSpec *param, ClockData *cd)
 {
-    location_update_ok_sensitivity (cd);
+        location_update_ok_sensitivity (cd);
 }

 static void

So pretty much consistent but for very specific places.

Anyway, using 8 spaces for a tab is too large.

Agreed, but I think patches should be consistent, and if indentation is to be changed that it's the only thing a patch changes. Actually the PR here is ever so slightly confusing because of this: it adds an outer block (if (display is X11) {…}) but doesn't change indentation of the body. Things like that are not a big issue, but can make understanding changes harder and reading the resulting file a tad harder as well.

[lukefromdc]

I respaced it to 8 spaces-but saved an older version of the file in case people didn't like the results.

[lukefromdc]

Converted to 4 spaces with a search and replace in pluma, which can handle working with spaces, tabs etc for this.

[cwendling]

Can also be easily reverted if you all want to save that for a later PR

Yes please, this makes the review very painful because you have to at best ignore the given commit, and then verify it separately. IMO this should really be another PR.

[lukefromdc]

Done, the 4 space reformatting is gone.

[lukefromdc]

rebased after https://github.com/mate-desktop/mate-panel/commit/5a7f770082568453f3a5f271a274156f08352101

[lukefromdc]

Thanks, I'll clean that up too when I get home. Pluma does NOT do a good job of flagging this, and not spoting the little things can make little problems that grow up to become big problems.

On 7/2/2023 at 7:13 AM, "raveit65" @.***> wrote:

@raveit65 commented on this pull request.

• gtk_window_move (GTK_WINDOW (cd->calendar_popup), x, y);
• gtk_window_set_gravity (GTK_WINDOW (cd->calendar_popup), gravity);
• break;
• }
• return;
• }

Line 1070 starts with a tab.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/mate- panel/pull/1377#pullrequestreview-1509545685 You are receiving this because you were mentioned.

Message ID: <mate-desktop/mate- @.***>

[raveit65]

@lukefromdc Pluma got an option to show tabs and spaces, very useful. Otherwise i wouldn't see that in resulting patch for rpm building.

Btw. i have wayfire running on bare metal, i can start testing all the wayland PRs now. Sadly in qemu VM, wayfire or gtk-layer-shell has glitches. Nothing fixed since my first tests with mate-panel some years ago. That means i have to logout from my regular x11 session, not comfortable but ok.

[lukefromdc]

I can switch to VT2 /3/whatever and run dbus-launch wayfire from there. You need dbus-launch for a lot of the MATE programs

[raveit65]

Thanks for the tip. Most applets from mate-panel and mate-applets are working more or less. I will post more details in mate-appelts PR. But the icons from panel-launchers are blurred in wayland. wayland:

compare to my x11 panel (HIDPI):

[raveit65]

Clock-applet works fine under wayfire. The window is exact at the same position like in X11 and behavior is the same. Good job.

[lukefromdc]

thanks for the tip about showing tabs and spaces, that will save me time and errors. Here's what I got, wonder if you were looking at an older version or something?

[raveit65]

I downloaded the resulting patch from PR with wget and i see the tab. Try wget https://patch-diff.githubusercontent.com/raw/mate-desktop/mate-panel/pull/1377.patch

[lukefromdc]

Only thing I can try is to redo the spaces there and force-push again. Strange, I just cloned the repo to the laptop from github though, this is odd

[lukefromdc]

When I retyped the spaces, git gui showed no change at all. Force-pushed anyway, if you still get a tab I have no idea what's causing that or how to fix it

[raveit65]

Forget this weird tab issue.

[lukefromdc]

I tested moving panels containing the clock, and moving the clock applet around, and didn't ever have a problem with the calendar getting "stretched" between the new and old location. This would be expected behavior if the calendar window is made anew each time it is shown. If it is not, adding the same fix from mate-media would ensure against problems down the road.

[lukefromdc]

Latest commit cleans up style. The "stretching" issue on orientation change turns out to exist if and only if the clock applet is moved while the calendar is showing in wayland, implying a new calendar window is being made each time it is shown. Simple fixes for that didn't work. Not sure how big a problem that is but I'd like to find a fix

[lukefromdc]

Latest commit ensure the calendar window can never be "stretched" across the screen no matter how you move the panel or applet even with the calendar window showing. When the calendar window is closed before moving the applet or panel there has never been a problem with incorrect rendering.

While the calendar window can sometimes render in the wrong position along the panel if the applet or panel are moved with the calendar window showing, it will always stay adjacent to the same panel the applet is on, and never go offscreen. We do get a runtime warning if and only if the panel or applet is moved with the calendar window showing, as this causes gtk-layer-shell settings to be changed on a widget that is mapped. Not sure how to fix that without destroying and remaking the calendar window. Running gtk_widget_unrealize before setting anchors and gtk_widget_realize() afterwards sometimes gives clean results, but sometimes causes a segfault. The behavior here though is considerably better than in x11, this is a case the calendar window probably was NOT designed to deal with

If the panel carrying the clock or the clock applet is moved with the calendar window showing in x11 (x11 codepath should be unchanged here) it always stays in the previous location. If just the applet is moved in x11 with the calendar showing, the calendar window moves with the applet but does not change orientation. That means moving the clock from a bottom to a top panel with the calendar showing in x11 reliably moves the calendar offscreen, with the visible edge over the applet showing it still exists.

Fixing that in x11 is a problem for another PR, the wayland behavior is now better than the x11 behavior.

[lukefromdc]

A couple of conversations on outdated code cannot be resolved and will have to be bypassed no matter what we do

[cwendling]

A couple of conversations on outdated code cannot be resolved and will have to be bypassed no matter what we do

I resolved them. You can usually find them in the conversations view somewhere, even when the Conversation panel on the changes tab can't find them.