Currently, a $flag has two properties, 'teaser' and 'node_form', that govern where the flag links (or checkboxes) are shown.

Since we now have an easy way to print the flag links, we can provide another property, 'page', that governs whether a link is shown on a 'page' node. This will allow the themer to turn it off and instead print the links himself in exactly the spot and manner he wishes.

Besides, the names of these three properties --that is, the database columns-- aren't very clear. I suggest we rename them to:

show_on_page
show_on_teaser
show_on_form

(Not 'show_on_node_form' because comments and users aren't nodes.)

(I think I borrowed the 'show_on_' prefix from nodequeue, but I'm not sure.)

On the settings page we could group these three checkboxes in a nice "Where to show the links?" fieldset.

Marking #216947: provide option to only display on node edit form a duplicate of this one.

Comments

mooffie’s picture

mooffie commented

show_on_page
show_on_teaser
show_on_form

(Not 'show_on_node_form' because comments and users aren't nodes.)

(Yeah, I know comments and users don't have a 'teaser' / 'page' notion either, so 'show_on_page' and 'show_on_teaser' aren't perfect names, but I can't think of better names right now.)

mooffie’s picture

mooffie commented
Title: Option to turn off flag links on 'page'; rename some DB columns » Option to turn off flag link on 'page'; rename some DB columns
mooffie’s picture

mooffie commented

I had a misunderstanding about a flag's content_type. I didn't know that a flag definition was _not_ associated with a certain content_type. So eventually we'll need to have more 'show_on_' columns, e.g. 'show_on_comment', 'show_on_user', etc... am I right?

quicksketch’s picture

quicksketch
he/him
commented

As I had planned it, flags could be applied on multiple content types. That is, you could make an "offensive" flag that applied to nodes, comments, and users all simultaneously. In theory, this would allow you to create a view using "flag_content" as the base table, and list all offensive content in a single view, regardless of the type of data being displayed.

Other than that, I thought it would be most convenient to be able to create a single flag that affected multiple types of content (and I only really had comment and node in mind, users? maybe...).

We could potentially make another whole table, for the contexts in which each would be applied. Is it a good idea, I'm not really sure. Then again, I'm not positive the database schema is perfect either. The most important thing is to get our API and theme functions solid. Anyone making database queries is asking for trouble anyway.

Alternatively, we could take the "easy" route and make an "options" column, that stores all "extra" data associated with flags. Considering flags are something I'd assume there'd be a very finite amount of, it's unlikely that users would want to query on where particular flags where being displayed.

quicksketch’s picture

quicksketch
he/him
commented

While setting up another flag today, I realized some more trouble areas we'll hit if we go this route of "one flag - many content types". Specifically using token-replacements doesn't work quite right any more, because users, comments, and nodes all have separate tokens. Maybe we should go the route you suggest and only have one type of content per flag.

mooffie’s picture

mooffie commented

Can you please give an example to a token that would fail?

Say we have a [flag-bookmark-count] token. It'd count all nodes+comments+users flagged. How, for example, would that fail?

mooffie’s picture

mooffie commented

We could have an optional "modifier" in our tokens, e.g. [flag-bookmark-count:node], [flag-bookmark-count:comment], [flag-bookmark-count:user].

I don't say it's a good idea, I'm just "brainstorming" here.

This idea, of associating a flag with different content_types is new for me, but perhaps it's a good thing after all. I haven't yet really thought about the ups and downs of it. I don't yet see how views could handle this, it's quite "revolutionary" for me at the moment :)

(I'm soon leaving the computer for a day and a half, so I won't be able to reply.)

mooffie’s picture

mooffie commented

I'm soon leaving the computer for [...]

But if you decide to change the module's behavior, of course feel free to go ahead and do it. I accept your judgement. After all, if I were to introduce this 'content_type', I wouldn't have thought to put it on the individual markings. Yet, I wonder, perhaps your idea was actually an extremely good one.

quicksketch’s picture

quicksketch
he/him
commented

I mean when setting up a flag, say you want the marked message to be "Bookmarked [title]". For comments this would need to be "Bookmarked [comment-title]" and user's would be "Bookmarked [user-name]".

Basically what I'm seeing here is less and less benefit to a single configuration screen, if it turns out that screen will need to be 3 times as long because we need to duplicate all the fields for each type.

mooffie’s picture

mooffie commented

"Bookmarked [title]". For comments this would need to be "Bookmarked [comment-title]"

Hmmm...... I see!

A tough decision. I'll leave it to you to decide.

(One day, though, I'd like to hear from you how you planned the 'multi content' Views support to go, it's interesting)

hafta go now...

quicksketch’s picture

quicksketch
he/him
commented

Just in case you sneak away to a computer... in the new Views, you can define any table as "base table" (like users, node, comment, etc already are by default). In one iteration of the Views integration I made "flag_content" a new base table, so you could generate a list of all flagged content, regardless of it's type. You'd need to make relationships out to nodes/users/comments to get additional information, but it was an interesting concept. A single list for all "offensive" content, regardless of it's type.

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Fixed

Alright, I thought back-and-forth about this quite a bit. First, I decided it absolutely necessary that we have a flexible system for storing type-specific options. Considering the amount of maintenance it's going to take to keep the upgrade path working from views_bookmark, I'd prefer to get the DB schema figured out so that we don't have to change it as long as people are upgrading.

So... this gives us two options: a new table for "flag_options" or a serialized "options" column. Either would provide type-specific options just fine, and the end-developer will never really deal with either, because they should always be using flag_get_flag() or flag_get_flags().

I decided to go with the serialized column because it's more efficient than a table join, and I think it's easier to get the values out of the database (no looping through rows and adding them to the flag object). The real benefit of a separate table would be ease of querying, which I don't think anyone will need anyway (they'll use flag_get_flags, like I said before).

So! After deciding that, I converted "node_form" and "teaser" columns into a single "options" column, and renamed them to "show_on_form", "show_on_teaser", and "show_on_page". Show on page was so easy at that point I added it also.

D5: http://drupal.org/cvs?commit=121481
D6: http://drupal.org/cvs?commit=121482

mooffie’s picture

mooffie commented
Version: » 6.x-1.0-beta1

decided to go with the serialized column

Excellent.

BTW, the 6 textual messages (flag_short, flag_long, etc. etc.) are good candidates for putting in 'options'. If we later add more sets of messages, we'll put them too in 'options' and things will be uniform.

==

As for 'show_on_form':

Since a flag may be associated with comments too, don't we need to name this setting 'show_on_node_form'? Because in the future we'll have 'show_on_comment_form', and having a vague 'show_on_form' at the same time will confuse programmers. (When I suggested 'show_on_form' it was before I realized the same flag may mark comments as well. Sorry for the bother.)

==

I see the following in the code:

$result = db_query("SELECT fid, show_on_form, teaser FROM {views_bookmarks}");
while ($bookmark = db_fetch_object($result)) {
  $options = array(
    'show_on_form' => $bookmark->show_on_form,
    'show_on_teaser' => $bookmark->teaser,
    'show_on_page' => 1

But I'm not sure the 'show_on_form' column exists in the {views_bookmarks} table at that point. Maybe I'm wrong.

==

Perhaps it's better to delete all files from HEAD (except a RAEDME.txt that explains where to find the code). I didn't notice you've branched a DRUPAL-6 and was preplexed when I didn't see your changes ;-)

==

Excellent work, Nate, your code is very professional. I've seen you've worked on Actions support as well, and will have a look at that too.

quicksketch’s picture

quicksketch
he/him
commented

BTW, the 6 textual messages (flag_short, flag_long, etc. etc.) are good candidates for putting in 'options'. If we later add more sets of messages, we'll put them too in 'options' and things will be uniform.

The purpose of the "options" field is to store things that may differ from content type to type. So display options make good sense, because we'll have different locations to display these things. Hopefully the 3 options of short, long, and message should suffice for any flag, but if they do need to vary by content type, then yes the options field would be fine.

Since a flag may be associated with comments too, don't we need to name this setting 'show_on_node_form'?

After our discussion and some more pondering, I felt that making a single flag support multiple types would not be worth it. So now there's a "content_type" column also in the flags table, though currently it only always defaults to 'node'.

I'm not sure the 'show_on_form' column exists in the {views_bookmarks} table at that point.

It should exist. VB in D5 uses "show_on_form". In the previous upgrade path, I'd been renaming "show_on_form" to "node_form". I removed that conversion and moved "show_on_form" directly into the options.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.