Opened 8 years ago

Last modified 8 years ago

#12519 new patch

Patch for "pounce when my status is..."

Reported by: grigoryj Owned by: darkrain42
Milestone: Patches Needing Improvement Component: libpurple
Version: 2.7.3 Keywords: pidgin libpurple pounce status
Cc:

Description

This patch adds a "pounce only when my status is..." option to a buddy pounce (see ticket #341). You can pick a specific status from a list of primitive status types (available/away/invisible/etc.) in the pounce editor dialog. Please let me know if anything needs to be changed before this patch can be included.

Cheers, Grigory Javadyan.

Attachments (2)

pounce_status.patch (13.5 KB) - added by grigoryj 8 years ago.
pounce_status_imporved.patch (14.0 KB) - added by grigoryj 8 years ago.

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by grigoryj

comment:1 Changed 8 years ago by datallah

  • Milestone set to Patches Needing Review

comment:2 Changed 8 years ago by salinasv

This patches add API so it can't be applied until a minor version release.

I'm not really familiar with status code so this needs a review form someone else but I will point you some things on your patch.

  • Avoid adding newlines, pounce.c:456, gtkpounce.c:1625
  • Don't use comments, pounce.c:1005
  • Fix the space to be consistent with the enum, pounce.h

Other than that, it looks ok to me. I haven't tested or anything just read it. =P

comment:3 follow-up: Changed 8 years ago by darkrain42

  • Component changed from unclassified to libpurple
  • Milestone changed from Patches Needing Review to Patches Needing Improvement
  • Owner rekkanoryo deleted

Beyond my comment on the mailing list (purple_debug_error -- oh, I see, you just matched the rest of the style in that code. That wasn't apparent from just the diff), some more (mostly coming in top->bottom order as I read it):

  • Use the name of a status, not the integral value of the enum. purple_primitive_get_id_from_type and purple_primitive_get_type_from_id :)

Haha, this parser code is still using GParser (that's not anything wrong with what you're doing, or something I expect you to change. I'm just amused :) )

  • "gtk_combo_box_get_active(GTK_COMBO_BOX(dialog->status)) + (PURPLE_STATUS_UNSET+1)" seems...evil, although I cannot think of a better way to do it OTOH (I'm also no GTK guru)
  • If you're going to export create_status_type_menu, rename it to match other exported functions (things in headers, a.k.a. pidgin_...thing).

More general issues:

  • If you're going to go the way of having generic fields in the pounce definition, make sure the parsing can handle multiple definitions of fields and values (I'm not sure it can now, but I'd have to look more closely). Also make the writing to disk generic (just write the name/value as strings -- hey, that also fits with what I said way way above!).

Design question: Would it make more sense to have a "Pounce if status is..." area, with tick boxes for each of the status primitives? That way I could poucne if someone is XA *or* Away, without needing two pounces.

comment:4 Changed 8 years ago by darkrain42

  • Owner set to darkrain42

comment:5 in reply to: ↑ 3 Changed 8 years ago by darkrain42

Replying to darkrain42:

  • "gtk_combo_box_get_active(GTK_COMBO_BOX(dialog->status)) + (PURPLE_STATUS_UNSET+1)" seems...evil, although I cannot think of a better way to do it OTOH (I'm also no GTK guru)

Okay, apparently that's used in gtksavedstatuses.c.

comment:6 Changed 8 years ago by grigoryj

Hello, I fixed the issues:

  • comment/newline issues
  • using the name of status instead of integral value
  • changed the name of create_status_types_menu to pidgin_create_status_types_menu

Concerning the other issues:

  • gtk_combo_box_get_active(GTK_COMBO_BOX(dialog->status)) + (PURPLE_STATUS_UNSET+1) - as you noticed a similar way is used in gtksavedstatuses.c, so I think this is good enough
  • About generic fields: currently my code generates the following XML:
    <fields>
     <field name='status' value='away'/>
    </fields>
    
    Isn't this generic enough?
  • About multiple pounce statuses: probably a good idea, but I'm not too comfortable writing GUI code. If anyone wishes to do the GUI part of this, I'll make necessary amendments to the libpurple part as quickly as possible.

I'm attaching the improved version of the patch. Thank you all very much for taking time to look at the code.

Cheers, Grigory

Changed 8 years ago by grigoryj

comment:7 Changed 8 years ago by darkrain42

  • Typo (the backtick):

+ xmlnode_set_attrib(child, "value", `purple_primitive_get_id_from_type(purple_pounce_get_status(pounce)));

  • With regard to the generic fields, I honestly just don't see the utility in it currently (I'm not quite sure what my complaint about genericness was specifically). Why not make the XML representation of the status something like:
    <only-when>
        <status name='away'/>
        <status name='available'/>
    </only-when>
    

Which I find clearer (even if we don't end up supporting multiple statuses).

comment:8 follow-up: Changed 8 years ago by grigoryj

  • I fixed the typo immediately - don't have a slightest idea how that backtick got there :)
  • Well, the next patch I'm going to write is "expirable buddy pounces" - so that you can set a date/time on which the pounce would expire. That would make use of generic fields as well. Like so:
     <fields>
      <field name = "status" value = "away"/>
      <field name = "expires" value = "20/08/2010 14:00"/>
     <fields>
    
    As for having multiple statuses, we can still use this xml format for them. Just add a new `status' field for each of the desired statuses and push them to a list as we parse the xml:
     <fields>
      <field name = "status" value = "away"/>
      <field name = "status" value = "extended away"/>
     <fields>
    
    I just don't like the idea of adding new constructions for each field type - that overcomplexifies the structure of the xml document and the parsing becomes more cumbersome.

comment:9 in reply to: ↑ 8 ; follow-ups: Changed 8 years ago by darkrain42

Replying to grigoryj:

  • I fixed the typo immediately - don't have a slightest idea how that backtick got there :)
  • Well, the next patch I'm going to write is "expirable buddy pounces" - so that you can set a date/time on which the pounce would expire. That would make use of generic fields as well. Like so:
     <fields>
      <field name = "status" value = "away"/>
      <field name = "expires" value = "20/08/2010 14:00"/>
    

Before I go any further, let me just say that date format is absolutely horrid. Either use something exactly like the format in XEP-0082: XMPP Date and Time Profiles or a UTC unix timestamp :)

<fields>

}}} As for having multiple statuses, we can still use this xml format for them. Just add a new `status' field for each of the desired statuses and push them to a list as we parse the xml:

 <fields>
  <field name = "status" value = "away"/>
  <field name = "status" value = "extended away"/>
 <fields>

I just don't like the idea of adding new constructions for each field type - that overcomplexifies the structure of the xml document and the parsing becomes more cumbersome.

The parsing there is already overly cumbersome -- it needs to be rewritten to use the xmlnode parser like all the other subsystems do. I'm still not entirely sure I understand what the current pounces parser is doing (or that your changes so far are correct!). So keep that in mind when designing a schema. In any event "fields" doesn't really describe something about what it's going to contain (in this case you seem to be aiming toward including filters/selectors on the event). Statuses should be all grouped together (look at accounts.xml), but not with things that aren't semantically similar.

I'm not sure what I'd do for the expiration time, but something like <validity start='YYYYMMDDThh:mm:ssTZD' end='YYYYMMDDThh:mm:ssTZD'/> or similar

comment:10 in reply to: ↑ 9 Changed 8 years ago by grigoryj

Replying to darkrain42:

Before I go any further, let me just say that date format is absolutely horrid. Either use something exactly like the format in XEP-0082: XMPP Date and Time Profiles or a UTC unix timestamp :)

Thanks, I'll take this into accout when writing that patch.

The parsing there is already overly cumbersome -- it needs to be rewritten to use the xmlnode parser like all the other subsystems do.

Well, that's not an excuse for making it even more ugly, is it?

I'm still not entirely sure I understand what the current pounces parser is doing (or that your changes so far are correct!). So keep that in mind when designing a schema. In any event "fields" doesn't really describe something about what it's going to contain (in this case you seem to be aiming toward including filters/selectors on the event). Statuses should be all grouped together (look at accounts.xml), but not with things that aren't semantically similar.

Well, we can group the statuses together without changing the format significantly (if I correctly understood what you mean, of course):

<fields>
 <field name="statuses" value="away,invisible"/>
</fields>

Again, adding new structural elements means even more ugly XML-parsing code. Plus, having a separate xml tag for each field type... well, just doesn't feel right. However, maybe "hierarchical" field structure is a way to go:

<fields>
 <field name="statuses">
  <field name="status" value="away"/>
  <field name="status" value="invisible"/>
 </field>
</fields>

This is a neat idea, IMO because it allows a more "uniform" parsing code.

comment:11 in reply to: ↑ 9 Changed 8 years ago by grigoryj

Hi again, I'd like to know what you think. If you think that hierarchical fields are a good idea, I'll implement it right away. I need more feeback to make the patch better :)

Thanks.

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!