From: Vassilii K. <vas...@ta...> - 2013-08-21 20:04:54
|
Hi fellow devs, in the scope of fixing bug# 6953, I have introduced an extra argument to BatchTool -- uistate, which you already know from the tool initialization API. Previously, tools would swallow uistate and BatchTool wouldn't know if there's any UI, then it would try to pop up a UI warning about an impossibility to undo the tool action, and then proceed. Now, if BatchTool sees there is no UI (and so it's run from the cmdline), it would just delegate to the base Tool class, and do nothing else. There is no UI to request any undo, so the user doesn't care about the warning anyway. If you own a 3rd-party tool that uses the BatchTool class, please update your tool accordingly. V. |
From: Nick H. <nic...@ho...> - 2013-08-21 20:19:41
|
I don't like this fix. We should pass an instance of the User class instead. Nick. On 21/08/13 21:04, Vassilii Khachaturov wrote: > Hi fellow devs, > > in the scope of fixing bug# 6953, I have introduced an extra argument to > BatchTool -- uistate, which you already know from the tool > initialization API. > > Previously, tools would swallow uistate and BatchTool wouldn't know if > there's any UI, then it would try to pop up a UI warning about an > impossibility to undo the tool action, and then proceed. > > Now, if BatchTool sees there is no UI (and so it's run from the > cmdline), it would just delegate to the base Tool class, and do nothing > else. There is no UI to request any undo, so the user doesn't care about > the warning anyway. > > If you own a 3rd-party tool that uses the BatchTool class, please update > your tool accordingly. > > V. > > ------------------------------------------------------------------------------ > Introducing Performance Central, a new site from SourceForge and > AppDynamics. Performance Central is your source for news, insights, > analysis and resources for efficient Application Performance Management. > Visit us today! > http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk > _______________________________________________ > Gramps-devel mailing list > Gra...@li... > https://lists.sourceforge.net/lists/listinfo/gramps-devel > > |
From: Doug B. <dou...@gm...> - 2013-08-21 20:28:01
|
Thanks, Vassilii, for the fix. But I think that Nick means that we should solve the problem like we do with Reports: pass in a User instance. Would that be an easy change? -Doug On Wed, Aug 21, 2013 at 4:19 PM, Nick Hall <nic...@ho...> wrote: > I don't like this fix. > > We should pass an instance of the User class instead. > > Nick. > > > On 21/08/13 21:04, Vassilii Khachaturov wrote: >> Hi fellow devs, >> >> in the scope of fixing bug# 6953, I have introduced an extra argument to >> BatchTool -- uistate, which you already know from the tool >> initialization API. >> >> Previously, tools would swallow uistate and BatchTool wouldn't know if >> there's any UI, then it would try to pop up a UI warning about an >> impossibility to undo the tool action, and then proceed. >> >> Now, if BatchTool sees there is no UI (and so it's run from the >> cmdline), it would just delegate to the base Tool class, and do nothing >> else. There is no UI to request any undo, so the user doesn't care about >> the warning anyway. >> >> If you own a 3rd-party tool that uses the BatchTool class, please update >> your tool accordingly. >> >> V. >> >> ------------------------------------------------------------------------------ >> Introducing Performance Central, a new site from SourceForge and >> AppDynamics. Performance Central is your source for news, insights, >> analysis and resources for efficient Application Performance Management. >> Visit us today! >> http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk >> _______________________________________________ >> Gramps-devel mailing list >> Gra...@li... >> https://lists.sourceforge.net/lists/listinfo/gramps-devel >> >> > > > ------------------------------------------------------------------------------ > Introducing Performance Central, a new site from SourceForge and > AppDynamics. Performance Central is your source for news, insights, > analysis and resources for efficient Application Performance Management. > Visit us today! > http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk > _______________________________________________ > Gramps-devel mailing list > Gra...@li... > https://lists.sourceforge.net/lists/listinfo/gramps-devel |
From: Doug B. <dou...@gm...> - 2013-08-21 23:44:06
|
On Wed, Aug 21, 2013 at 7:28 PM, Vassilii Khachaturov <vas...@ta...> wrote: > Hold on a second! > > I agree we should add User to Tool API, but I don't think we should pass it > to BatchTool in the current scheme of things. > > This is because User today is for abstracting the difference between CLI and > GUI. My change is not about sending the interaction of BatchTool with the > user to CLI instead of GUI. It's about not being interactive at all because > there is no GUI and hence no undo capability is available no matter if the > tool is a batch one or not. Ah, that makes sense. Then we should mention somewhere that having a uistate == None means that it should run without user interaction. We may need that in the Reports as well. > So, if BatchTool doesn't get uistate, but gets only user, I see the > following options to achieve the objective: > 1) use constfunc.has_display() rather than uistate (ugly global access, but > allows to revert adding the uistate to BatchTool completely, and untangles > #6953 from #5598) I don't like that, because it should be up to the user/program to decide how it should be run. For example, it may be that you have a DISPLAY, but you want to quietly run a tool in the background. > 2) extend the User interface to allow querying for whether there's GUI -- in > effect, pass uistate as an attribute implemented in the GUI/CLI User > subclass > 3) use issubclass(user, gui.User) to actually deduce uistate thus encoded in > the user arg (the most perverted option to my taste) > > Am I missing something? Shouldn't user just be an attribute of uistate, if there is one? -Doug > If not, I'll revert the additional uistate argument from BatchTool and take > option (1) as the least intrusive; #5598 will still happen, but there's no > rush any more. > > V. > > > On 22.08.2013 01:19, Vassilii Khachaturov wrote: >> >> I totally agree this should be the next step, as requested in #5598. >> >> This doesn't seem to be a difficult change, but it will be slightly more >> effort than the current fix. >> >> To preserve compatibility with the existing Tool subclasses, I shall add a >> user=gen.user.User() as the last argument of Tool.__init__(), and so 3rd >> party tools won't be broken. >> >> I'll then change the BatchTool uistate argument to a user as well, but I >> am afraid in this case it's not a good idea to have a default "no-op" -- if >> somebody subclassed BatchTool it was precisely to have the warning >> displayed! So for BatchTool subclasses, the 3rd party tool developers will >> still have to change stuff. >> >> Any objections to this plan? >> >> V. >> >> On 21.08.2013 23:27, Doug Blank wrote: >>> >>> Thanks, Vassilii, for the fix. But I think that Nick means that we >>> should solve the problem like we do with Reports: pass in a User >>> instance. Would that be an easy change? >>> >>> -Doug >>> >>> On Wed, Aug 21, 2013 at 4:19 PM, Nick Hall <nic...@ho...> >>> wrote: >>>> >>>> I don't like this fix. >>>> >>>> We should pass an instance of the User class instead. >>>> >>>> Nick. >>>> >>>> >>>> On 21/08/13 21:04, Vassilii Khachaturov wrote: >>>>> >>>>> Hi fellow devs, >>>>> >>>>> in the scope of fixing bug# 6953, I have introduced an extra argument >>>>> to >>>>> BatchTool -- uistate, which you already know from the tool >>>>> initialization API. >>>>> >>>>> Previously, tools would swallow uistate and BatchTool wouldn't know if >>>>> there's any UI, then it would try to pop up a UI warning about an >>>>> impossibility to undo the tool action, and then proceed. >>>>> >>>>> Now, if BatchTool sees there is no UI (and so it's run from the >>>>> cmdline), it would just delegate to the base Tool class, and do nothing >>>>> else. There is no UI to request any undo, so the user doesn't care >>>>> about >>>>> the warning anyway. >>>>> >>>>> If you own a 3rd-party tool that uses the BatchTool class, please >>>>> update >>>>> your tool accordingly. >>>>> >>>>> V. > > |
From: Doug B. <dou...@gm...> - 2013-08-22 12:51:38
|
On Thu, Aug 22, 2013 at 6:02 AM, Nick Hall <nic...@ho...> wrote: > On 22/08/13 00:43, Doug Blank wrote: >>> >>> Am I missing something? >> >> Shouldn't user just be an attribute of uistate, if there is one? > > > It should be the other way around. The uistate could be an attribute of the > gui User class, if required. Actually, looking at reports and User, User has all that we need (eg, progress, dialogs). > The point is that interaction with the user should use the User class. Yes, indeed. All that we are using uistate in tools should be replaced with User methods. But perhaps we need to use ./gramps/gen/user.py as a no-op User when there should be no interaction. So there are these cases: 1) cli with interaction 2) cli with no interaction 3) gui For example, you don't want an automated backup process asking any questions. It might have an error, but should not ask questions. Should we add a cli flag to differentiate between 1 and 2? -Doug > In the batch tool example, the same method should be called in both the cli > and gui modes. The gui will display a dialog and ask for user confirmation, > whilst the cli doesn't require input from the user. > > Nick. > |
From: Nick H. <nic...@ho...> - 2013-08-22 14:03:52
|
On 22/08/13 13:51, Doug Blank wrote: > So there are these cases: > > 1) cli with interaction > 2) cli with no interaction > 3) gui > > For example, you don't want an automated backup process asking any > questions. It might have an error, but should not ask questions. > > Should we add a cli flag to differentiate between 1 and 2? At the moment we don't have an option for cli with interaction. The prompt method does actually so anything in cli mode. Errors, warnings and information is output to stderr in cli mode, rather than using a dialog. The main output, for example from an export, is output to stdout. I also think that the User class has all that we need. Nick. |
From: Doug B. <dou...@gm...> - 2013-08-22 14:25:25
|
On Thu, Aug 22, 2013 at 10:03 AM, Nick Hall <nic...@ho...> wrote: > On 22/08/13 13:51, Doug Blank wrote: >> >> So there are these cases: >> >> 1) cli with interaction >> 2) cli with no interaction >> 3) gui >> >> For example, you don't want an automated backup process asking any >> questions. It might have an error, but should not ask questions. >> >> Should we add a cli flag to differentiate between 1 and 2? > > > At the moment we don't have an option for cli with interaction. The prompt > method does actually so anything in cli mode. Oh, you are right: in ./gramps/cli/user.py the prompt(title, question) method always returns False. That could be very dangerous, depending on how the question is written: "Do you want to save to a new destination? (False will overwrite)" We should have a flag to provide the current behavior (respond False to all questions), otherwise it should use standard IO to ask and get response. Then we can make sure that Tools/Reports are written in the appropriate manner. > Errors, warnings and information is output to stderr in cli mode, rather > than using a dialog. The main output, for example from an export, is output > to stdout. > > I also think that the User class has all that we need. Agreed. -Doug > Nick. > |
From: Nick H. <nic...@ho...> - 2013-08-22 16:33:50
|
On 22/08/13 15:25, Doug Blank wrote: > Oh, you are right: in ./gramps/cli/user.py the prompt(title, question) > method always returns False. That could be very dangerous, depending > on how the question is written: > > "Do you want to save to a new destination? (False will overwrite)" > > We should have a flag to provide the current behavior (respond False > to all questions), otherwise it should use standard IO to ask and get > response. Then we can make sure that Tools/Reports are written in the > appropriate manner. Yes, that seems like a good approach. Perhaps something like a -y flag to answer 'yes' to all questions? Nick. |
From: Vassilii K. <vas...@ta...> - 2013-08-22 20:35:49
|
On 22.08.2013 19:33, Nick Hall wrote: > On 22/08/13 15:25, Doug Blank wrote: >> Oh, you are right: in ./gramps/cli/user.py the prompt(title, question) >> method always returns False. That could be very dangerous, depending >> on how the question is written: >> >> "Do you want to save to a new destination? (False will overwrite)" Actually, User.prompt looks like a stub everywhere -- it is the same implementation in gen, cli, and gui... Right now User encapsulates output to user, and only in "progress reporting" and "warn/error" scenario. While at it, shouldn't we change the signature of User.prompt() to more closely match one of the QuestionDialog2, in line with how the warn/error are modeled on WarnDialog and ErrorDialog, as we implement it for real? We have a free reign because BatchTool will be the very first instance actually using it. It will help preserve the strings without need to rewrite them. Returning "False" is sensible in this context as it means the user has rejected the choice. >> We should have a flag to provide the current behavior (respond False Based on what I have said above, I believe we should return "True" not "False", in case the new flag is on. >> to all questions), otherwise it should use standard IO to ask and get >> response. Then we can make sure that Tools/Reports are written in the >> appropriate manner. > > Yes, that seems like a good approach. > > Perhaps something like a -y flag to answer 'yes' to all questions? > > Nick. > I'd vote for '-q' / '--quiet' --- accept the default answer to all questions and suppress progress indication. V. |
From: Nick H. <nic...@ho...> - 2013-08-22 21:35:25
|
On 22/08/13 21:35, Vassilii Khachaturov wrote: > While at it, shouldn't we change the signature of User.prompt() to > more closely match one of the QuestionDialog2, in line with how the > warn/error are modeled on WarnDialog and ErrorDialog, as we implement > it for real? We have a free reign because BatchTool will be the very > first instance actually using it. It will help preserve the strings > without need to rewrite them. Yes. That seems sensible. > Based on what I have said above, I believe we should return "True" not > "False", in case the new flag is on. Yes. These prompts are likely to be for "Are you sure?" type questions. In which case, we would need to return "True" in non-interactive mode. > > I'd vote for '-q' / '--quiet' --- accept the default answer to all > questions and suppress progress indication. I'm happy with that. Nick. |
From: John R. <jr...@ce...> - 2013-08-23 03:41:28
|
On Aug 22, 2013, at 2:35 PM, Nick Hall <nic...@ho...> wrote: > On 22/08/13 21:35, Vassilii Khachaturov wrote: > >> >> I'd vote for '-q' / '--quiet' --- accept the default answer to all >> questions and suppress progress indication. > > I'm happy with that. > I think -y|--yes (like fsck) would be less surprising. Regards, John Ralls |
From: Vassilii K. <vas...@ta...> - 2013-08-23 07:27:05
|
On 23.08.2013 06:41, John Ralls wrote: > On Aug 22, 2013, at 2:35 PM, Nick Hall <nic...@ho...> wrote: > >> On 22/08/13 21:35, Vassilii Khachaturov wrote: >> >>> I'd vote for '-q' / '--quiet' --- accept the default answer to all >>> questions and suppress progress indication. >> I'm happy with that. >> > I think -y|--yes (like fsck) would be less surprising. > > Regards, > John Ralls > Do you mean that user's expectation from "-q / --quiet" from other programs is most likely to be output suppression only? V |
From: John R. <jr...@ce...> - 2013-08-23 13:47:20
|
On Aug 23, 2013, at 12:26 AM, Vassilii Khachaturov <vas...@ta...> wrote: > On 23.08.2013 06:41, John Ralls wrote: >> On Aug 22, 2013, at 2:35 PM, Nick Hall <nic...@ho...> wrote: >> >>> On 22/08/13 21:35, Vassilii Khachaturov wrote: >>> >>>> I'd vote for '-q' / '--quiet' --- accept the default answer to all >>>> questions and suppress progress indication. >>> I'm happy with that. >>> >> I think -y|--yes (like fsck) would be less surprising. >> >> Regards, >> John Ralls >> > Do you mean that user's expectation from "-q / --quiet" from other programs is most likely to be output suppression only? Yes. Regards, John Ralls |
From: Nick H. <nic...@ho...> - 2013-08-23 12:42:15
|
On 23/08/13 08:26, Vassilii Khachaturov wrote: > On 23.08.2013 06:41, John Ralls wrote: >> On Aug 22, 2013, at 2:35 PM, Nick Hall <nic...@ho...> wrote: >> >>> On 22/08/13 21:35, Vassilii Khachaturov wrote: >>> >>>> I'd vote for '-q' / '--quiet' --- accept the default answer to all >>>> questions and suppress progress indication. >>> I'm happy with that. >>> >> I think -y|--yes (like fsck) would be less surprising. >> >> Regards, >> John Ralls >> > Do you mean that user's expectation from "-q / --quiet" from other > programs is most likely to be output suppression only? > > V > > John is correct. The -y option would be the most appropriate. http://www.faqs.org/docs/artu/ch10s05.html Windows also uses /y to suppress confirmation. http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/copy.mspx Nick. |
From: Vassilii K. <vas...@ta...> - 2013-08-23 13:46:23
|
Thanks. So, the summary so far is (also posted in #5898 additional information): 1) Replace the currently unused User.prompt stub with a method based on QuestionDialog2 interface instead ({gen|cli|gui}.User) 2) Add a new cmdline switch -y --yes for cmdline unattended execution of cli.User.prompt(), that would force the 1st (default) answer w/o asking the user 3) Replace the uistate with user in Tool and BatchTool signature. 4) Also (lower prio.) add a new cmdline switch -q --quiet to suppress extra diagnostic during unattended runs, such as progress indicator. Let's now elaborate on (3) -- how to best do it across the branches. The only thing that remains to decide is the scope of the change in (3). Right now a lot of tools expect uistate argument in this position, and have, inside them, various code that eventually should be converted to calls via User. This conversion is a major work that I think should be done as a second stage. 3.1) As the first stage, I propose to select an approach that would enable leaving the numerous legacy "if uistate..." code intact. Tools.__init__ signature should be changed to take a user parameter in this position, and then accept a boolean argument for initialization of self.user attribute. Legacy tool scenario: If a boolean uistate is given, it should do smth like self.user = gen.user.create_user(uistate). New tool scenario: Otherwise, it should use the passed user object. 3.2) In BatchTool, we remove uistate and add user as the last positional argument with a default value of None, which will work as in the legacy scenario above. 3.3) once this works on trunk, we back-port to gramps40 and gramps34 3.4) Then, as a second stage (trunk only!), we can convert the legacy tools from code conditioned on uistate and directly using the UI dialogs/progress meter to code delegating to self.user. OK? V. |
From: Doug B. <dou...@gm...> - 2013-08-23 15:44:06
|
On Fri, Aug 23, 2013 at 9:46 AM, Vassilii Khachaturov <vas...@ta...> wrote: > Thanks. So, the summary so far is (also posted in #5898 additional > information): Vassilli, great work! This is something that has been needed to be done for a long time. Thanks for taking it on. > 1) Replace the currently unused User.prompt stub with a method based on > QuestionDialog2 interface instead ({gen|cli|gui}.User) > 2) Add a new cmdline switch -y --yes for cmdline unattended execution of > cli.User.prompt(), that would force the 1st (default) answer w/o asking > the user > 3) Replace the uistate with user in Tool and BatchTool signature. > 4) Also (lower prio.) add a new cmdline switch -q --quiet to suppress > extra diagnostic during unattended runs, such as progress indicator. That all sounds good. You can probably deal with the flags -q and -y at the same time, as they are probably both settings to handle in the User instance (the first doesn't print anything, the second doesn't ask anything). I don't think I would backport these changes though. Typically, we only backport bug fixes, and this is well outside of that. I would just add this to trunk, and not worry about stages. I think backporting would have too large an impact. -Doug > > Let's now elaborate on (3) -- how to best do it across the branches. > > The only thing that remains to decide is the scope of the change in (3). > Right now a lot of tools expect uistate argument in this position, and > have, inside them, various code that eventually should be converted to > calls via User. This conversion is a major work that I think should be > done as a second stage. > > 3.1) As the first stage, I propose to select an approach that would > enable leaving the numerous legacy "if uistate..." code intact. > Tools.__init__ signature should be changed to take a user parameter in > this position, and then accept a boolean argument for initialization of > self.user attribute. > Legacy tool scenario: If a boolean uistate is given, it should do smth > like self.user = gen.user.create_user(uistate). > New tool scenario: Otherwise, it should use the passed user object. > > 3.2) In BatchTool, we remove uistate and add user as the last positional > argument with a default value of None, which will work as in the legacy > scenario above. > > 3.3) once this works on trunk, we back-port to gramps40 and gramps34 > > 3.4) Then, as a second stage (trunk only!), we can convert the legacy > tools from code conditioned on uistate and directly using the UI > dialogs/progress meter to code delegating to self.user. > > OK? > > V. > > ------------------------------------------------------------------------------ > Introducing Performance Central, a new site from SourceForge and > AppDynamics. Performance Central is your source for news, insights, > analysis and resources for efficient Application Performance Management. > Visit us today! > http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk > _______________________________________________ > Gramps-devel mailing list > Gra...@li... > https://lists.sourceforge.net/lists/listinfo/gramps-devel |
From: Vassilii K. <vas...@ta...> - 2013-08-23 16:23:41
|
On 23.08.2013 18:44, Doug Blank wrote: > On Fri, Aug 23, 2013 at 9:46 AM, Vassilii Khachaturov > <vas...@ta...> wrote: >> Thanks. So, the summary so far is (also posted in #5898 additional >> information): > Vassilli, great work! This is something that has been needed to be > done for a long time. Thanks for taking it on. Thank you for your kind words! >> 1) Replace the currently unused User.prompt stub with a method based on >> QuestionDialog2 interface instead ({gen|cli|gui}.User) >> 2) Add a new cmdline switch -y --yes for cmdline unattended execution of >> cli.User.prompt(), that would force the 1st (default) answer w/o asking >> the user >> 3) Replace the uistate with user in Tool and BatchTool signature. >> 4) Also (lower prio.) add a new cmdline switch -q --quiet to suppress >> extra diagnostic during unattended runs, such as progress indicator. > That all sounds good. You can probably deal with the flags -q and -y > at the same time, as they are probably both settings to handle in the > User instance (the first doesn't print anything, the second doesn't > ask anything). > > I don't think I would backport these changes though. Typically, we > only backport bug fixes, and this is well outside of that. I would > just add this to trunk, and not worry about stages. I think > backporting would have too large an impact. In any case, the incremental plan for (3) makes sense to organize the work in a continuous way, committing things to the trunk as I go along and have working test coverage. Once the API change is in place, and before we begin to convert the legacy tools, we can decide if it is risky to back-port or not. Given the excellent UT infrastructure revival work by Nick, I'm confident you'll be less worried as (3.2) is done! V. > -Doug > >> Let's now elaborate on (3) -- how to best do it across the branches. >> >> The only thing that remains to decide is the scope of the change in (3). >> Right now a lot of tools expect uistate argument in this position, and >> have, inside them, various code that eventually should be converted to >> calls via User. This conversion is a major work that I think should be >> done as a second stage. >> >> 3.1) As the first stage, I propose to select an approach that would >> enable leaving the numerous legacy "if uistate..." code intact. >> Tools.__init__ signature should be changed to take a user parameter in >> this position, and then accept a boolean argument for initialization of >> self.user attribute. >> Legacy tool scenario: If a boolean uistate is given, it should do smth >> like self.user = gen.user.create_user(uistate). >> New tool scenario: Otherwise, it should use the passed user object. >> >> 3.2) In BatchTool, we remove uistate and add user as the last positional >> argument with a default value of None, which will work as in the legacy >> scenario above. >> >> 3.3) once this works on trunk, we back-port to gramps40 and gramps34 >> >> 3.4) Then, as a second stage (trunk only!), we can convert the legacy >> tools from code conditioned on uistate and directly using the UI >> dialogs/progress meter to code delegating to self.user. >> >> OK? >> >> V. >> >> ------------------------------------------------------------------------------ >> Introducing Performance Central, a new site from SourceForge and >> AppDynamics. Performance Central is your source for news, insights, >> analysis and resources for efficient Application Performance Management. >> Visit us today! >> http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk >> _______________________________________________ >> Gramps-devel mailing list >> Gra...@li... >> https://lists.sourceforge.net/lists/listinfo/gramps-devel |
From: Vassilii K. <vas...@ta...> - 2013-08-29 14:19:43
|
>>> 1) Replace the currently unused User.prompt stub with a method based on >>> QuestionDialog2 interface instead ({gen|cli|gui}.User) >>> 2) Add a new cmdline switch -y --yes for cmdline unattended >>> execution of >>> cli.User.prompt(), that would force the 1st (default) answer w/o asking >>> the user >>> 3) Also (lower prio.) add a new cmdline switch -q --quiet to suppress >>> extra diagnostic during unattended runs, such as progress indicator. The above has been completed, fully tested, and back-ported to gramps40, where it was needed for me to fix another bug #7021. Let me take this opportunity to thank Nick again for his excellent work in reviving the UT framework, which allowed me to do it with confidence! >>> 4) Replace the uistate with user in Tool and BatchTool signature. >>> >> [SNIP] The plan for the remaining (major!) part (with the proposed incremental breakdown that can be found in #5598 "Additional Info") was based on a wrong assumption on how tools are built --- actually, the Tool.__init__ doesn't take uistate, it's the actual tool class (as specified in the tool.gpr.py) that has an __init__ taking uistate; all tool invocations happen from either tool.cli_tool or tool.gui_tool helper. This means I can't just easily change a common base of all tools and get handle of the user object there :( So I'll have to think of another plan... V. |
From: Nick H. <nic...@ho...> - 2013-08-29 21:53:32
|
On 29/08/13 15:19, Vassilii Khachaturov wrote: > The plan for the remaining (major!) part (with the proposed > incremental breakdown that can be found in #5598 "Additional Info") > was based on a wrong assumption on how tools are built --- actually, > the Tool.__init__ doesn't take uistate, it's the actual tool class (as > specified in the tool.gpr.py) that has an __init__ taking uistate; all > tool invocations happen from either tool.cli_tool or tool.gui_tool > helper. This means I can't just easily change a common base of all > tools and get handle of the user object there :( > > So I'll have to think of another plan... > There seem to be two types of tool: 1. Tools that can be run from either CLI or GUI. These require little user interaction, and should be passed a User instance rather than a DipslayState (uistate). Any tool that can be run from the command line should also be available in the GUI. 2. Tools that require user interaction. These use a ManagedWindow to present an interface to the user. The ManagedWindow will need a uistate. There are actually a couple of classes in gui/plug/_windows.py for such tools: ToolManagedWindow and ToolManagedWindowBatch. Only one tool actually makes use of them though. Perhaps we should re-think how we use the tool modes? There are two modes: TOOL_MODE_GUI and TOOL_MODE_CLI. We could change the definition so that a tool can have only one mode. A CLI tool could be passed a User instance. A GUI tool would be passed a uistate, requiring no change. Nick. |
From: Vassilii K. <vas...@ta...> - 2013-08-31 17:06:39
|
On 30.08.2013 00:53, Nick Hall wrote: > 1. Tools that can be run from either CLI or GUI. > > These require little user interaction, and should be passed a User > instance rather than a DipslayState (uistate). Any tool that can be > run from the command line should also be available in the GUI. > > 2. Tools that require user interaction. > > These use a ManagedWindow to present an interface to the user. The > ManagedWindow will need a uistate. > > There are actually a couple of classes in gui/plug/_windows.py for > such tools: ToolManagedWindow and ToolManagedWindowBatch. Only one > tool actually makes use of them though. > > Perhaps we should re-think how we use the tool modes? There are two > modes: TOOL_MODE_GUI and TOOL_MODE_CLI. We could change the > definition so that a tool can have only one mode. > > A CLI tool could be passed a User instance. A GUI tool would be > passed a uistate, requiring no change. Maybe we should let uistate be a property of the gui User class, like discussed earlier in this thread. Then we can unify the signature for both kind of tools, and have the GUI tools extract uistate from it. Or maybe we should have gui User inherit from DisplayState, in this case there is an easier way to treat the legacy code. BTW, I propose to move to keyword-only syntax of calling plugins, so that future upgrades to the core/3rd-party interface can be more seamless. V. |
From: Vassilii K. <vas...@ta...> - 2013-09-04 14:22:07
|
On 31.08.2013 20:06, Vassilii Khachaturov wrote: > On 30.08.2013 00:53, Nick Hall wrote: >> 1. Tools that can be run from either CLI or GUI. >> >> These require little user interaction, and should be passed a User >> instance rather than a DipslayState (uistate). Any tool that can be >> run from the command line should also be available in the GUI. >> >> 2. Tools that require user interaction. >> >> These use a ManagedWindow to present an interface to the user. The >> ManagedWindow will need a uistate. >> >> There are actually a couple of classes in gui/plug/_windows.py for >> such tools: ToolManagedWindow and ToolManagedWindowBatch. Only one >> tool actually makes use of them though. >> >> Perhaps we should re-think how we use the tool modes? There are two >> modes: TOOL_MODE_GUI and TOOL_MODE_CLI. We could change the >> definition so that a tool can have only one mode. >> >> A CLI tool could be passed a User instance. A GUI tool would be >> passed a uistate, requiring no change. > Maybe we should let uistate be a property of the gui User class, like > discussed earlier in this thread. Then we can unify the signature for > both kind of tools, and have the GUI tools extract uistate from it. Or > maybe we should have gui User inherit from DisplayState, in this case > there is an easier way to treat the legacy code. > > BTW, I propose to move to keyword-only syntax of calling plugins, so > that future upgrades to the core/3rd-party interface can be more seamless. > > V. > Does anybody have any objections against trying the above approach on trunk? I have already committed supprot for the gui User to store a displaystate inside. To actually enable this, I am going to add user to the session manager (stored as CLIManager.user), which would be sometimes passed as gui User with DisplayState inside from the gui ViewManager. Then, as the next step, I will change the tools' classes signature to accept user instead of uistate. V |
From: Nick H. <nic...@ho...> - 2013-09-04 17:21:48
|
On 04/09/13 15:21, Vassilii Khachaturov wrote: > On 31.08.2013 20:06, Vassilii Khachaturov wrote: >> On 30.08.2013 00:53, Nick Hall wrote: >>> 1. Tools that can be run from either CLI or GUI. >>> >>> These require little user interaction, and should be passed a User >>> instance rather than a DipslayState (uistate). Any tool that can be >>> run from the command line should also be available in the GUI. >>> >>> 2. Tools that require user interaction. >>> >>> These use a ManagedWindow to present an interface to the user. The >>> ManagedWindow will need a uistate. >>> >>> There are actually a couple of classes in gui/plug/_windows.py for >>> such tools: ToolManagedWindow and ToolManagedWindowBatch. Only one >>> tool actually makes use of them though. >>> >>> Perhaps we should re-think how we use the tool modes? There are two >>> modes: TOOL_MODE_GUI and TOOL_MODE_CLI. We could change the >>> definition so that a tool can have only one mode. >>> >>> A CLI tool could be passed a User instance. A GUI tool would be >>> passed a uistate, requiring no change. >> Maybe we should let uistate be a property of the gui User class, like >> discussed earlier in this thread. Then we can unify the signature for >> both kind of tools, and have the GUI tools extract uistate from it. Or >> maybe we should have gui User inherit from DisplayState, in this case >> there is an easier way to treat the legacy code. >> >> BTW, I propose to move to keyword-only syntax of calling plugins, so >> that future upgrades to the core/3rd-party interface can be more >> seamless. >> >> V. >> > Does anybody have any objections against trying the above approach on > trunk? > I have already committed supprot for the gui User to store a > displaystate inside. > > To actually enable this, I am going to add user to the session manager > (stored as CLIManager.user), > which would be sometimes passed as gui User with DisplayState inside > from the gui ViewManager. > > Then, as the next step, I will change the tools' classes signature to > accept user instead of uistate. > > V > > About two-thirds of the tools are GUI-only, and inherit from ManagedWindow. I don't see any point in changing these - they can continue to be passed the DisplayState (uistate). The tools that can be run from the command line should be passed a User instance instead of DisplayState. These tools should be simple to modify. If a User instance contains a DisplayState, it should really be private to the class and not accessed by the tools directly. Nick. |
From: Vassilii K. <vas...@ta...> - 2013-08-21 22:19:34
|
I totally agree this should be the next step, as requested in #5598. This doesn't seem to be a difficult change, but it will be slightly more effort than the current fix. To preserve compatibility with the existing Tool subclasses, I shall add a user=gen.user.User() as the last argument of Tool.__init__(), and so 3rd party tools won't be broken. I'll then change the BatchTool uistate argument to a user as well, but I am afraid in this case it's not a good idea to have a default "no-op" -- if somebody subclassed BatchTool it was precisely to have the warning displayed! So for BatchTool subclasses, the 3rd party tool developers will still have to change stuff. Any objections to this plan? V. On 21.08.2013 23:27, Doug Blank wrote: > Thanks, Vassilii, for the fix. But I think that Nick means that we > should solve the problem like we do with Reports: pass in a User > instance. Would that be an easy change? > > -Doug > > On Wed, Aug 21, 2013 at 4:19 PM, Nick Hall <nic...@ho...> wrote: >> I don't like this fix. >> >> We should pass an instance of the User class instead. >> >> Nick. >> >> >> On 21/08/13 21:04, Vassilii Khachaturov wrote: >>> Hi fellow devs, >>> >>> in the scope of fixing bug# 6953, I have introduced an extra argument to >>> BatchTool -- uistate, which you already know from the tool >>> initialization API. >>> >>> Previously, tools would swallow uistate and BatchTool wouldn't know if >>> there's any UI, then it would try to pop up a UI warning about an >>> impossibility to undo the tool action, and then proceed. >>> >>> Now, if BatchTool sees there is no UI (and so it's run from the >>> cmdline), it would just delegate to the base Tool class, and do nothing >>> else. There is no UI to request any undo, so the user doesn't care about >>> the warning anyway. >>> >>> If you own a 3rd-party tool that uses the BatchTool class, please update >>> your tool accordingly. >>> >>> V. >>> >>> ------------------------------------------------------------------------------ >>> Introducing Performance Central, a new site from SourceForge and >>> AppDynamics. Performance Central is your source for news, insights, >>> analysis and resources for efficient Application Performance Management. >>> Visit us today! >>> http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk >>> _______________________________________________ >>> Gramps-devel mailing list >>> Gra...@li... >>> https://lists.sourceforge.net/lists/listinfo/gramps-devel >>> >>> >> >> ------------------------------------------------------------------------------ >> Introducing Performance Central, a new site from SourceForge and >> AppDynamics. Performance Central is your source for news, insights, >> analysis and resources for efficient Application Performance Management. >> Visit us today! >> http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk >> _______________________________________________ >> Gramps-devel mailing list >> Gra...@li... >> https://lists.sourceforge.net/lists/listinfo/gramps-devel > ------------------------------------------------------------------------------ > Introducing Performance Central, a new site from SourceForge and > AppDynamics. Performance Central is your source for news, insights, > analysis and resources for efficient Application Performance Management. > Visit us today! > http://pubads.g.doubleclick.net/gampad/clk?id=48897511&iu=/4140/ostg.clktrk > _______________________________________________ > Gramps-devel mailing list > Gra...@li... > https://lists.sourceforge.net/lists/listinfo/gramps-devel |
From: Vassilii K. <vas...@ta...> - 2013-09-04 18:32:29
|
On 04.09.2013 20:42, Doug Blank wrote: > On Wed, Sep 4, 2013 at 1:21 PM, Nick Hall <nic...@ho...> wrote: >> On 04/09/13 15:21, Vassilii Khachaturov wrote: >>> On 31.08.2013 20:06, Vassilii Khachaturov wrote: >>>> On 30.08.2013 00:53, Nick Hall wrote: >>>>> 1. Tools that can be run from either CLI or GUI. >>>>> >>>>> These require little user interaction, and should be passed a User >>>>> instance rather than a DipslayState (uistate). Any tool that can be >>>>> run from the command line should also be available in the GUI. >>>>> >>>>> 2. Tools that require user interaction. >>>>> >>>>> These use a ManagedWindow to present an interface to the user. The >>>>> ManagedWindow will need a uistate. >>>>> >>>>> There are actually a couple of classes in gui/plug/_windows.py for >>>>> such tools: ToolManagedWindow and ToolManagedWindowBatch. Only one >>>>> tool actually makes use of them though. >>>>> >>>>> Perhaps we should re-think how we use the tool modes? There are two >>>>> modes: TOOL_MODE_GUI and TOOL_MODE_CLI. We could change the >>>>> definition so that a tool can have only one mode. >>>>> >>>>> A CLI tool could be passed a User instance. A GUI tool would be >>>>> passed a uistate, requiring no change. >>>> Maybe we should let uistate be a property of the gui User class, like >>>> discussed earlier in this thread. Then we can unify the signature for >>>> both kind of tools, and have the GUI tools extract uistate from it. Or >>>> maybe we should have gui User inherit from DisplayState, in this case >>>> there is an easier way to treat the legacy code. >>>> >>>> BTW, I propose to move to keyword-only syntax of calling plugins, so >>>> that future upgrades to the core/3rd-party interface can be more >>>> seamless. >>>> >>>> V. >>>> >>> Does anybody have any objections against trying the above approach on >>> trunk? >>> I have already committed supprot for the gui User to store a displaystate >>> inside. >>> >>> To actually enable this, I am going to add user to the session manager >>> (stored as CLIManager.user), >>> which would be sometimes passed as gui User with DisplayState inside from >>> the gui ViewManager. >>> >>> Then, as the next step, I will change the tools' classes signature to >>> accept user instead of uistate. >>> >>> V >>> >>> >> About two-thirds of the tools are GUI-only, and inherit from ManagedWindow. >> I don't see any point in changing these - they can continue to be passed the >> DisplayState (uistate). At the first stage of the refactoring, those tools will begin with uistate = user.uistate so nobody should be upset about it much. > I haven't thought about this much, but perhaps one reason to change > them is that they could then be tested using the new testing system. That was, of course, part of my secret future plans :-) I thought we could be attacking the tools one-by-one and gently nudge them to better testability and CLI-friendliness. > Another reason might be that with a different User class, perhaps it > could be used via the web. That is a very cool idea I haven't thought of! > > I guess it is the case that these tools can't be used via the command > line interface (CLI)? It might be handy if they could, even if it just > worked with the defaults. Does the CLI options allow one to set the > options for a tool? If so, then they would be totally functional via > the CLI (and thus the web). > > -Doug > Hear, hear! V. |
From: Nick H. <nic...@ho...> - 2013-09-04 22:06:14
|
On 04/09/13 19:32, Vassilii Khachaturov wrote: >>> About two-thirds of the tools are GUI-only, and inherit from >>> ManagedWindow. >>> I don't see any point in changing these - they can continue to be >>> passed the >>> DisplayState (uistate). > At the first stage of the refactoring, those tools will begin with > uistate = user.uistate > so nobody should be upset about it much. The User class should not make uistate directly available to the tool. Instead, it should provide functionality for different interface components through its methods. These methods can then be implemented for cli, gui (and possibly web) versions. Although a few tools might be complex, I think you will find common components in most tools. Other more interactive tools may be better implemented as gramplets. Nick. |