[00:52] teward: yes that works. I like that it is much easier to read and easy to make sure there are spaces where they need to be without adding +" "+ kinds of junk. [00:53] I have found another bug though :( I will look at fixing that. [01:05] yep. this is why I like format strings :P [01:05] OvenWerks: and what bug, if I may ask? [01:29] the visible check box was not working right (just pushed fix) I could make something not visible, but could not make it visible again. And I had to do an edit to do even that. [01:31] teward: ^^ So I have fixed that. (running glade to add one small change sure makes for a big diff) [01:34] OvenWerks: I take it that was not my fault then :p [01:34] no not your falt [01:37] OvenWerks: I just triggered a rebuild. [01:47] Eickmeyer: might be a while :Po [01:47] I can fast build if you want [01:48] teward: I'm in no rush. [01:48] Dunno about OvenWerks [01:49] well this also lets me notice any odd new lintian warns :p [01:49] teward: Also, looks like it's already built and just needs to be publish. [01:49] teward: True. Go ahead. [01:50] yep looks good. [01:50] not sure if hte publisher will be fast enough :P [01:50] but i'm impatient :) [01:51] https://people.ubuntu.com/~teward/ubuntustudio-menu-add_0.1_all_v4.deb if you are as impatient as I :) [01:51] glad to hear though my little change there didn't blow it up :) [01:51] i thought for a second I had introduced another bug xD [01:53] Nah, looks like you're good. Any problems that came from that have been circumvented, I take it? [01:54] Eickmeyer: AIUI yes [01:54] OW said there was a bug in the visible checkbox not working right [01:54] but not related to my code [01:54] and I think OW is now in love with format strings xD [01:54] I see that. Looks like he fixed it. [01:54] hehehehe [01:54] both f-strings [01:54] and "".format :P [01:55] Maybe someday I'll learn Python and know what that means. [01:56] heh [02:10] Eickmeyer: basically? [02:10] Huh? [02:10] instead of doing "some string "+variable+" was here" it's easier to know where things are in the string [02:11] f"some string {variable} was here" <-- easier to make sure things are exactly where they need to be [02:11] AND to quote out entire args [02:11] Okay, that makes sense. [02:11] I had to turn that into a bash script in my head. [02:12] it's not actaully that far off ;) [14:51] Eickmeyer: f-strings are like super bash instring variables as the "variables" can be equasions or the return of functions [16:17] OvenWerks: can we confirm the bugs you ID'd are fixed and documented in the changelog and 'ready for upload'? [16:17] and that 'this works'? [16:17] once that's ready I can responsor it [16:57] teward: they bugs are fixed... running my local file before uploading. I will DL the package and check this afternoon to check that way too. [16:58] teward: in many ways I would like other people to try it as that exposes my blind spots... [16:59] but we seems to have very few people willing to do that. [17:00] When I build something, everything inside is "intuitive" to me... but I tend to be the odd one out in many of these things [17:12] OvenWerks: well I don't run Studio [17:13] so make Eickmeyer test it :p [17:13] * teward is just here as a 'helper' today [17:25] * Eickmeyer tested it already [17:26] teward: I'd go ahead. If we can get it into Eoan, I should have upload rights for bugfixes, or at least can get them from the DMB. [17:27] If we can get people testing the dailies, that would be the best route at this point. [17:27] * Eickmeyer is going crazy applying for jobs. [17:44] ack [17:44] i will pull latest, debuild, and upload again. If it rejects I'll let you know. [17:44] teward: ack [17:47] this is missing the subprocess patches... [17:47] the one that switches the subprocess call to array based [17:47] OvenWerks: did that get pushed? [17:47] or did we expect me to apply that? [17:48] or did that still not work? [17:50] the change which does subprocess.run(["/usr/bin/exo-desktop-item-edit", str(self.tempfile)], shell=False) instead of the string based exec [17:50] Eickmeyer: ^ this was the last bit that sarnold wanted done [17:50] but it was untested [17:50] 'cause i'm not on eoan [17:50] nor STudio [17:51] so if this still doesn't work i'll be happy to leave it as is with the strings [17:51] but sarnold wanted it array-based preferably [17:51] rather than string-driven [17:51] OvenWerks needs to address that then. [18:11] ack [18:16] I thought that had been changed. sorry, I will look at it. [18:17] I thought that was what I tested. [18:19] https://git.launchpad.net/ubuntustudio-menu-add/tree/usr/bin/ubuntustudio-menu-add?id=bd332ecd04cf778bd60208dbd187dd9658eaf7b4 [18:20] teward: ^ [18:20] line 418 [18:20] OvenWerks: that's the format-string driven one [18:20] i had another patch hang on [18:20] that applied it as an *array* with the full path [18:20] guess it never pushed [18:20] 'cause i didn't hit push [18:21] I notice the terminal =true too :( [18:21] yeah that's why it's not got my code in it. Mine uses shell=False [18:21] and sarnold doesn't like shell=True if it can be avoided [18:21] for security reasons [18:22] okay... [18:22] https://git.launchpad.net/ubuntustudio-menu-add/commit/?id=50791c079560173736ec8eaaa2dc4362b297c0e0 [18:22] which uses the array [18:22] lets see if *this* works. if not we'll revert [18:22] this pulls the full path in so [18:22] if this works as-is then all is good [18:24] it SHOULD but it'll need a test ;) [18:27] teward: that works here.... so what is different? When we tried it before it failed. Maybe shell == yes needs string? [18:27] OvenWerks: no [18:27] it's shell=False [18:27] taht's what we needed [18:27] which i discovered during some tests [18:27] Yes, I agree [18:28] I was wondering why it failed before (yesterday) [18:28] yeah i was headscratching [18:28] then i did some local Python subprocess call tests [18:28] and found it [18:33] The only differences I see are " rather than ', full path and Shell=false [18:34] I think the shell=False could be omitted as being default but leaving it in is self documenting. [18:34] teward: ^^ [18:35] yeah I left it in there for explicitly making sure. I used double quotes by accident, but from Python's perspective 'foobar' == "foobar" as they're both strings [18:35] if you prefer single quotes that's a minor change [18:35] Not worth it [18:35] mmkay. I need to add some changelog entries since I contributed now :| [18:36] I tend toward double quotes... not sure why (K&R c maybe?) [18:36] *SHRUGS* [18:36] oops caps [18:38] OK so I also added to the changelog since it has my two revisions per sarnold [18:38] sarnold: want to do one last review before this gets reeval'd for sponsoring? [18:39] sure [18:40] https://git.launchpad.net/ubuntustudio-menu-add/tree/usr/bin/ubuntustudio-menu-add should be less offensive wrt subprocess and lib calls :) [18:43] btw is there a reason why this line uses concatenation? [18:43] self.tempfile = str(os.path.expanduser("~/"))+".config/ubuntustudio_menu_temp.desktop" [18:43] sarnold: because OvenWerks hasn't gotten to using an f-string for it yet [18:43] hmm I guess I could just try .. [18:43] since they only learned about format strings yesterday-ish(?) [18:44] os.path.expanduser("~/.does/not/exist") [18:44] '/home/sarnold/.does/not/exist' [18:44] no need for foramt strings [18:44] sarnold: the concat is to add the rest. [18:44] that's why the concat as is works [18:45] and yes i'm annoying asking sarnold to spotcheck [18:45] but since sarnold caught the first batch of errors [18:45] i'm being cautious :) [18:46] oh OvenWerks just a question [18:46] self.menufolder in menu_cb [18:46] that's not init'd in __init__ [18:46] should it be? [18:46] even if it's init'd to None? [18:46] (code inspection shows it as a low-level consideration) [18:46] (not blocking but still a question) [18:48] teward: well, it looks like os.path.expanduser() doesn't need the file to exist, it should work fine without the concatenation or format string or hwatever [18:48] indeed, it doesn't. [18:48] sarnold: so you're saying straight os.path.expanduser("~/.config/ubuntustudio_menu_temp.desktop") ? [18:48] (as an example) [18:49] yeah that should work, right? [18:50] yep even without the str-casting since it returns a str [18:50] yay [18:51] anything else glaring? [18:51] Eickmeyer: OvenWerks: and forgive our bootprints over this :) [18:52] nope, thanks for fixing up the executions to use arrays :) [18:52] yeah that was a PITA to trace [18:53] OvenWerks: at your convenience, `git pull` and test please :) [18:53] if all still works [18:53] i'll sponsor it up :) [18:57] teward, sarnold: I think I speak for both of us when I say any improvements are welcoe. [18:57] *welcome [18:57] indeed. [18:57] Eickmeyer: of course, sarnold does A LOT of code review so spots things I sometimes don't :) [18:57] Makes sense. [18:58] everyone spots different things :) [18:58] I'm good at noticing things along syscall boundaries [18:58] and sometimes algorithmic mistakes [18:58] other people know specific idioms in languages better [18:59] teward: self.menufolder is actually initialized in the glade file. [18:59] but now time to watch some women's world cup :) have fun [18:59] OvenWerks: ah. ack, OK. [18:59] just thought I'd ask :) [18:59] OvenWerks: if everything works fine here though as-is then this is Good 2 Go and I'll be happy to upload (unless you want a test build too) [18:59] the data is static unlike submenu [19:00] I am still testing, I looked at the init first [19:03] I din't know I could give the whole string for expanduser. Thank you. Yes that still works. [19:04] teward: thinking about logically, it does make sense I could do it that way. [19:05] * OvenWerks wonders how ubuntustudio-controls got through... [19:05] OvenWerks: Probably because it was an already-existing binary. [19:07] I will go through -controls sometime fixing some of these things there as well. [19:08] The lines of code count should go down. [19:08] OvenWerks: probably because it didn't have me or sarnold digging into the code :P [19:08] (my coredev is new as of this week and Eickmeyer asked me to poke so :P) [19:09] teward: I will at least go through the code first before we sak for a look. [19:10] 👍 [19:10] I should be able to get rid of things like shell=True [19:10] OvenWerks: yeah, since shell=True is already insecure [19:10] they even say it in the docs xD [19:10] thanks for the effort though, just make sure it works with the array execution and those strings being expanded [19:10] if all looks good I'll kick it into NEW queue [19:11] yup, but it is obvious to me my main problem there is using the shell to find the executable, just give the path and that can go away [19:11] teward: ubuntustudio-menu-add looks good to me. anything those changes affect has been tested. [19:12] ack [19:12] are you and Eickmeyer comfortable with this being 0.1 in the repos then? [19:12] yes [19:13] when I work on -controls, I will commit each kind of change. (maybe tab to space frst) [19:13] good idea :) [19:14] * OvenWerks is off to lunch [20:49] OvenWerks: Eickmeyer: just as an FYI: the package was accepted to the NEW queue as is now [20:49] it's waiting AA review [20:49] at their convenience. [20:50] * teward goes to pack up before heading home from work