[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Request to review and upload unhide 20210124-2



Hello Samuel,

Thanks for your advice on the translation support in the previous email.
With that help, I managed to polish the draft source package posted a few weeks ago.
I'd appreciate it if you take a look at my revised changes:
https://salsa.debian.org/dfukui/unhide/-/commits/debian/master-rebase

To help us understand how things have changed since the last draft, 
I'd like to show how each of the issues you mentioned was addressed as follows. 

> To make the review easier and avoiding possible conflicts, I suggest
> that you rebase your changes on the main repo, I have done this on my
> side for now but it's a good practice that will save you from a
> headache:
I can't agree with you more.
Rebasing a branch would help us understand how things have changed since the latest commit.
Please note that the branch shown above has been rebased.

> I recommend changing the "Copyright" part of the patch in favor of a
> DEP3 header, you can use "Author":
> https://dep-team.pages.debian.net/deps/dep3/
Following this example [0], the header parts of the patches I created were modified like these:

https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Fix-wrong-path-for-python3-interpreter.patch
https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Translate-French-texts-into-English.patch
https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Use-paths-defined-in-unhide.install.patch

[0] https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/allocate-pid-arrays-from-heap.patch

> I got the time to checkout unhideGui in more detail now:
> It looks like parts of it are in French, with no support for translations:
> https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L291
> https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L294
> https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L303
All French texts in the script were translated into English except for comment lines:
https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/patches/Translate-French-texts-into-English.patch

Hopefully I covered "all" French texts...

> 1) desktop file:
> For GUI applications it's always a good idea to provide a .desktop
> file, so users can start it from their DEs. But in the case of unhide,
> the GUI would need to change to identify when it's not running as root
> and call polkit (or something similar) to get the privileges.
Yes, this is a good idea.
As an experiment, I tried to use pkexec when calling unhide-gui, but that attempt ended up with the following error.

$ pkexec /usr/sbin/unhide-gui
Traceback (most recent call last):
  File "/usr/sbin/unhide-gui", line 325, in <module>
    root=Tk(className = "UnhideGUI")
  File "/usr/lib/python3.8/tkinter/__init__.py", line 2270, in __init__
    self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects, useTk, sync, use)
_tkinter.TclError: no display name and no $DISPLAY environment variable

According to the manpage of pkexec [1], "pkexec will not allow you to run X11 applications as another user since the $DISPLAY and $XAUTHORITY environment variables are not set".
Thus it looks like pkexec would not be suitable for our use case.
I'm now searching for an alternative way to implement our idea, but it may take some time or end up with no luck in the worst case.
For now, I'd like to skip adding the .desktop file to our package.

[1] https://manpages.debian.org/unstable/pkexec/pkexec.1.en.html

> 2) unhideGui:
> I think the name of the binary could be improved to try to follow the
> pattern we usually see on unix tools (and in the other binaries of
> unhide), maybe unhide-gui (all lowercase)?
Yes, that would help users find the tool.
See:
https://salsa.debian.org/dfukui/unhide/-/blob/debian/master-rebase/debian/unhide.links

> 3) commit:
> There's just a single commit which is adding the GUI, the ToolTip and
> a few other things:
> https://salsa.debian.org/dfukui/unhide/-/commit/b095ddb22e6c1ad3d403f7ae0c244b3a1a76cc36
That's correct. A huge commit could prevent us from understanding how things were changed.
The commit you pointed at were divided into two parts:
https://salsa.debian.org/dfukui/unhide/-/commit/44e3d8986dc9cc570d074835a8714c03ad0fd15e
https://salsa.debian.org/dfukui/unhide/-/commit/574c759bb695fe21e2f20fbb18c544adde2cf5b1

> 3.1) add-new-filename-ToolTip.py.patch:
> I don't think this patch is needed, as "tar_list" is only used by
> upstream to generate the tarball.
This patch file was removed.

> 3.2) allocate-pid-arrays-from-heap.patch:
> The changes to these patch were probably done automatically by some
> tool, they're not needed as the patch already has the DEP-3 headers.
You're correct.
It looks like the changes had been mistakenly added to the previous draft.
Now they were dropped.

I really appreciate your comments on my updates and your patience for  my slow response.
Hopefully these changes make sense and are helpful.

P.S. 
Salsa CI for unhide fails at random due to this issue:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1012035
After running the reprotest job several times, it finally passed.

Best regards,
Fukui

Reply to: