Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/#review23954 --- This review has been submitted with commit 7a99d5708f9ba2bedbc197c740f8b64f81c6f575 by Jekyll Wu to branch KDE/4.9. - Commit Hook On Dec. 9, 2012, 7:50 a.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Dec. 9, 2012, 7:50 a.m.) Review request for kdelibs, Plasma and Luboš Luňák. Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), the patch now works with 1), 2) and 4) . I don't know whether it make senses to support all combinations . This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs - ksmserver/server.cpp a65b35a Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/#review23955 --- This review has been submitted with commit c7fc5ed96143d91e16787a7e231f2c87e9fa2df5 by Jekyll Wu to branch KDE/4.10. - Commit Hook On Dec. 9, 2012, 7:50 a.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Dec. 9, 2012, 7:50 a.m.) Review request for kdelibs, Plasma and Luboš Luňák. Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), the patch now works with 1), 2) and 4) . I don't know whether it make senses to support all combinations . This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs - ksmserver/server.cpp a65b35a Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
On Nov. 11, 2012, 11:13 a.m., Aaron J. Seigo wrote: ksmserver/server.cpp, line 939 http://git.reviewboard.kde.org/r/107276/diff/1/?file=94565#file94565line939 why not check for both program and filename? that should then catch 1, 2 and 4, no? excludeApps could also be pre-processed to include both long paths and filenames which would then allow catching all 4 variations. probably the reason this was written to only catch 1 and 4, however, was in case there were binaries of the same name in different paths that should be treated differently (allowing differentiation by full path).. Lubos will certainly have more insight on this, however. probably the reason this was written to only catch 1 and 4, however, was in case there were binaries of the same name in different paths that should be treated differently (allowing differentiation by full path).. I think that is a valid case, but I also think it is rare. I prefer to first fix this more common problem with a simple patch, and wait to see how many users will notice and complain this rare case not working any more. - Jekyll --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/#review21816 --- On Dec. 9, 2012, 7:50 a.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Dec. 9, 2012, 7:50 a.m.) Review request for kdelibs, Plasma and Luboš Luňák. Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), the patch now works with 1), 2) and 4) . I don't know whether it make senses to support all combinations . This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs - ksmserver/server.cpp a65b35a Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Dec. 9, 2012, 7:50 a.m.) Review request for kdelibs, Plasma and Luboš Luňák. Changes --- update the patch to also work with condition 4) update the description to reflect the patch change Description (updated) --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), the patch now works with 1), 2) and 4) . I don't know whether it make senses to support all combinations . This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs (updated) - ksmserver/server.cpp a65b35a Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/#review23084 --- Sorry for the late reply, I'm not active in KDE anymore (and so I guess this means ksmserver is unmaintained). The patch looks ok, but it makes sense to support 4) as well, so please add || excludeApps.contains( program.toLower()) too and then it's ok. Supporting 3) is most probably a bad idea. - Luboš Luňák On Nov. 10, 2012, 12:45 p.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Nov. 10, 2012, 12:45 p.m.) Review request for kdelibs, Plasma and Luboš Luňák. Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), this patch works with 1) and 2) . I don't know whether it make senses to support all combinations This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs - ksmserver/server.cpp eb3ac18 Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/#review21816 --- ksmserver/server.cpp http://git.reviewboard.kde.org/r/107276/#comment16844 why not check for both program and filename? that should then catch 1, 2 and 4, no? excludeApps could also be pre-processed to include both long paths and filenames which would then allow catching all 4 variations. probably the reason this was written to only catch 1 and 4, however, was in case there were binaries of the same name in different paths that should be treated differently (allowing differentiation by full path).. Lubos will certainly have more insight on this, however. - Aaron J. Seigo On Nov. 10, 2012, 12:45 p.m., Jekyll Wu wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Nov. 10, 2012, 12:45 p.m.) Review request for kdelibs, Plasma and Luboš Luňák. Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), this patch works with 1) and 2) . I don't know whether it make senses to support all combinations This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs - ksmserver/server.cpp eb3ac18 Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Make sure ksmserver ignores excluded apps when saving session
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107276/ --- (Updated Nov. 10, 2012, 12:45 p.m.) Review request for kdelibs, Plasma and Luboš Luňák. Changes --- Forgot uploading the patch Description --- It is easy to understand why the existing code (usually) fails: * Users are most likely to just specify short names, like dolphin,gwenview,okular,rekonq, instead of /usr/bin/konsole,/usr/bin/gwenview,/usr/bin/okular,/usr/bin/rekonq * When ksmserver saves the session, it usually gets the full names, like /usr/bin/dolphin, unless you have started that dolphin instance by typing dolphin exactly in a shell. So there are four possible combinations : 1). config uses short name, runtime gets short name (this guy starts everything from konsole, never using kio/krun) 2). config uses short name, runtime gets long name (I think this is the most common one) 3). config uses long name, runtime gets short name 4). config uses long name, runtime gets long name (I guess some users use this combination because they find only that way works after trying various workaround...) The existing code works with 1) and 4), this patch works with 1) and 2) . I don't know whether it make senses to support all combinations This addresses bug 242760. http://bugs.kde.org/show_bug.cgi?id=242760 Diffs (updated) - ksmserver/server.cpp eb3ac18 Diff: http://git.reviewboard.kde.org/r/107276/diff/ Testing --- Thanks, Jekyll Wu ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
