Hi,

Thanks for the patch. Let's inspect this patch. 

I need your feed back before applying this patch.

On Sun, Mar 22, 2015 at 10:02:51AM +0800, Aron Xu wrote:
> Package: src:im-config
> Severity: wishlist
> 
> The upstart user session job that makes im-config works under it.
> Upstart is still used as user session init on Ubuntu, even though the
> system init has been changed to systemd recently. There's still no
> intention of removing upstart from Debian, so this could be still
> useful.
> 
> The attached job file works even when the display server isn't X
> (hence Xsession stuff aren't run). The file needs to be installed to
> /usr/share/upstart/sessions/im-config.conf
> 
> Thanks,
> Aron

This looks interesting though I have no idea about the non-X display
server starting mechanism used on Ubuntu.  This script may need some
touch up to be perfect and this discussion may be useful for the
upcoming Wayland support.

Please read the following first to be sure. 

The sysV init Xsession starting mechanism does 2 things.
 * PHASE=1 (70im-config_launch)
   Export environment variables to the child processes so they can find
   appropriate input methods:
     * XMODIFIERS sets it for plain XIM (via X IPC to IM daemon)
     * GTK*_IM_MODULE sets it for GTK*  (via library)
     * QT*_IM_MODULE sets it for QT*  (via library)

 * PHASE=2 (im-launch)
   Start the IM daemon including GUI indicator apps at the end of
   Xsession startup process.

   The IM daemon starting hook im-launch is specified in the STARTUP
   shell variable.

   The Xsession startup sequence updates the STARTUP shell variable as:
        STARTUP="$<NEW_PROGRAM_NAME> $STARTUP"
   so $<NEW_PROGRAM_NAME> is parent of previously defined $STARTUP .

   This ensures dbus started by 75dbus_dbus-launch etc. are ready to be
   used to avoid some race conditions previously reported for GUI
   indicator apps started too early.  Basically, this $STARTUP string is
   used to start programs in the reverse order of Xsession.d sequence.

Let me make comments on proposed patch code (prefixed with | ):

| start on starting xsession-init and started dbus

OK,  So you want this after dbus when starting xsession-init.
 
As I discuss below, GPG-agent and SSH-agent may need to be waited if
they exist.  What is your thought?

| pre-start script
|     . /etc/X11/Xsession.d/70im-config_launch

Since IM_CONFIG_PHASE should have been undefined, this should define it
as 1 while exporting environment variables if no other script touched on
them in advance.

|     if [ "$IM_CONFIG_PHASE" = 1 ]; then

Can I reorder your script as follows so it is more logical to me.
(If there is some reason why it was placed at the end, let me know)  

I assume Upstart does not honor export unless specifically told to
export via "initctl set-env ...".  So you added these for Upstart.  By
putting within this "if ..." segment, you wisely ensured no other script
touched on them in advance.

|         initctl set-env -gr XMODIFIERS=$XMODIFIERS
|         initctl set-env -gr GTK_IM_MODULE=$GTK_IM_MODULE
|         initctl set-env -gr QT_IM_MODULE=$QT_IM_MODULE
|         initctl set-env -gr QT4_IM_MODULE=$QT4_IM_MODULE
|         initctl set-env -gr CLUTTER_IM_MODULE=$CLUTTER_IM_MODULE

Since STARTUP variable is not used on Ubuntu Upstart case, I guess you
pasted im-launch as below to be run as a part of this initialization to
start daemons.

Question is is dbus enough to be waited?  GPG and SSH agent also mangles
STARTUP.

|         IM_CONFIG_PHASE=2
|         # initialize all im-config common functions and parameters
|         . /usr/share/im-config/xinputrc.common

Can I update this script to prevent the leaky environment variables by
adding the followings (See https://bugs.debian.org/767483 and 0.29-1):
>         unset TEXTDOMAIN
>         unset TEXTDOMAINDIR

|         # source the first found configuration file
|         if [ -r "$IM_CONFIG_XINPUTRC_USR" ]; then
|             . $IM_CONFIG_XINPUTRC_USR
|         elif [ -r "$IM_CONFIG_XINPUTRC_SYS" ]; then
|             . $IM_CONFIG_XINPUTRC_SYS
|         fi
| 
|     fi
| end script

Regards,

Osamu


-- 
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Reply via email to